bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file

[Matt Armstrong]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: craven@gmx.net, 46397@debbugs.gnu.org, Matt Armstrong <gmatta@gmail.com>
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Wed, 10 Feb 2021 11:23:46 -0800
>> 
>> --- a/src/filelock.c
>> +++ b/src/filelock.c
>> @@ -532,7 +532,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
>>    /* If nonexistent lock file, all is well; otherwise, got strange error. */
>>    lfinfolen = read_lock_data (lfname, owner->user);
>>    if (lfinfolen < 0)
>> -    return errno == ENOENT ? 0 : errno;
>> +    return errno == ENOENT || errno == ENOTDIR ? 0 : errno;
>
> As I said, I don't think this is TRT.  Why should we silently ignore
> ENOTDIR? couldn't it mean some kind of malicious attack on the user's
> account, or some other trouble?
>
> We should not ignore these errors, we should ask the user what to do
> about them.  The user can tell us the error can be ignored, but we
> should not decide that without asking.

I think Paul's commit is a good one. I'll try to explain why.

The commit does not silently ignore ENOTDIR. Instead, it is explicitly
handles that particular error code it in a way that honors the lock file
API contract.

In this case, Paul's commit changes the current_lock_owner() function
such that it returns zero upon ENOTDIR. The caller must interpret the
zero return as meaning "at the time current_lock_owner() was called,
nobody owned the lock file...or the lock file was obsolete."

ENOTDIR has a specific meaning that we can rely on. Both ENOENT and
ENOTDIR imply that the file was definitely not on disk at the time of
the call. Because of this, current_lock_owner() can safely conclude that
nobody owned the lock.

Put another way, I think a clearer API spec for current_lock_owner()
would be:

/* Return -2 if the current process owns the lock file,

   or -1 if another process appears to own it (and set OWNER (if
       non-null) to info),

   or 0 if the lock file isn't there, or it is there but has no current
       owner,

   or an errno value if something is wrong with the locking mechanism. */

That spec is semantically equivalent to the current one, but makes it
more clear why ENOENT and ENOTDIR should cause the function to return
zero. Those errno values do not imply that "something is wrong with the
locking mechanism."

Up a level of abstraction, the callers of the lock file API are still
presented with coherent semantics, even if current_lock_owner() returns
"lock file not owned" when the directory is missing. If the higher level
code wants to acquire the lock, it will attempt that and get an error at
that time. If the higher level code wants to abandon a buffer, it can
just do that knowing that this process definitely doesn't own that lock.
In neither case the user need not be interrogated, and if they do, it
will be done at a time that makes much more sense to them -- when
actually trying to acquire the lock.