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

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong-3
"Peter" <[hidden email]> writes:

> The following steps reproduce this:
> - Make sure /tmp/tmp does not exist
> - Open a buffer /tmp/tmp/foo.txt
> - Make some changes to the buffer.
> - Do *not* save the buffer or create the directory /tmp/tmp/
> - Create /tmp/tmp as a *file* (not a directory)
> - Try to kill the buffer.
>
> You will see the following error message:
>
> Unlocking file: Not a directory, /tmp/tmp/foo.txt
>
> I just want to kill the buffer, I don't want to save it.
>
> This is easily rectified by just deleting / moving away /tmp/tmp, but it
> seems like this could be resolved to just delete the buffer.

I confirmed this in 27.1 and the master branch. I also found it
difficult to exit Emacs once this state was reached. If you choose "no"
when saving modified buffers Emacs hits this error and is left in a
strange state (there are no displayed buffers or mode line, but Emacs is
still running).

The backtrace is unsurprising:

Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
  kill-buffer("test.txt")
  funcall-interactively(kill-buffer "test.txt")
  call-interactively(kill-buffer nil nil)
  command-execute(kill-buffer)



Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong
Matt Armstrong <[hidden email]> writes:

> "Peter" <[hidden email]> writes:
>
>> The following steps reproduce this:
>> - Make sure /tmp/tmp does not exist
>> - Open a buffer /tmp/tmp/foo.txt
>> - Make some changes to the buffer.
>> - Do *not* save the buffer or create the directory /tmp/tmp/
>> - Create /tmp/tmp as a *file* (not a directory)
>> - Try to kill the buffer.
>>
>> You will see the following error message:
>>
>> Unlocking file: Not a directory, /tmp/tmp/foo.txt
>>
>> I just want to kill the buffer, I don't want to save it.
[...]

> The backtrace is unsurprising:
>
> Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
>   kill-buffer("test.txt")
>   funcall-interactively(kill-buffer "test.txt")
>   call-interactively(kill-buffer nil nil)
>   command-execute(kill-buffer)

I found that this behavior was introduced by Paul Egger's commit
9dc306b1db0, discussed in Bug#37389.  I've cc'd Paul.

Paul's commit changed unlock_file() (from src/filelock.cc) to report
errors from unlink(), excempting only ENOENT. This bug demonstrates a
way to induce an ENOTDIR error. I've attached a patch that ignores
ENOTDIR as well, which is the most conservative fix I can think of. It
also seems in-line with Paul's original intent, since he was saying that
both ENOENT and ENOTDIR are usually "tame."


From fc0b7f2595bd8680952f062d2dd5261f94394e1c Mon Sep 17 00:00:00 2001
From: Matt Armstrong <[hidden email]>
Date: Tue, 9 Feb 2021 16:14:28 -0800
Subject: [PATCH] Ignore ENOTDIR errors from unlink().

* src/filelock.c (unlock_file): Ignore ENOTDIR errors from unlink().
---
 src/filelock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/filelock.c b/src/filelock.c
index 35baa0c666..af5683f365 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -731,7 +731,8 @@ unlock_file (Lisp_Object fn)
   MAKE_LOCK_NAME (lfname, fn);
 
   int err = current_lock_owner (0, lfname);
-  if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
+  if (err == -2 && unlink (lfname) != 0
+      && (errno != ENOENT && errno != ENOTDIR))
     err = errno;
   if (0 < err)
     report_file_errno ("Unlocking file", filename, err);
--
2.30.0

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Matt Armstrong <[hidden email]>
> Date: Tue, 09 Feb 2021 16:23:09 -0800
> Cc: [hidden email], Paul Eggert <[hidden email]>
>
> > The backtrace is unsurprising:
> >
> > Debugger entered--Lisp error: (file-error "Unlocking file" "Not a directory" "/private/tmp/tmp/test.txt")
> >   kill-buffer("test.txt")
> >   funcall-interactively(kill-buffer "test.txt")
> >   call-interactively(kill-buffer nil nil)
> >   command-execute(kill-buffer)
>
> I found that this behavior was introduced by Paul Egger's commit
> 9dc306b1db0, discussed in Bug#37389.  I've cc'd Paul.
>
> Paul's commit changed unlock_file() (from src/filelock.cc) to report
> errors from unlink(), excempting only ENOENT. This bug demonstrates a
> way to induce an ENOTDIR error. I've attached a patch that ignores
> ENOTDIR as well, which is the most conservative fix I can think of. It
> also seems in-line with Paul's original intent, since he was saying that
> both ENOENT and ENOTDIR are usually "tame."

I think instead of ignoring some errors, we should allow the user to
get out of these situations, after showing the error.  That is,
instead of silently ignoring the error on some low level, we should
propagate it to userlock.el and allow the user to decide whether
he/she wants to ignore the error or do something about that.  These
errors could mean something innocent, or they could mean something
more serious, and we shouldn't second-guess which one is it.

Letting the user decide will also seamlessly solve any other type of
error that could happen when we try to delete the lock file.

So how about coming up with a patch that shows the error message to
the users and asks them whether or not to ignore that and kill the
buffer?

Thanks.



Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
On 2/10/21 7:05 AM, Eli Zaretskii wrote:

> I think instead of ignoring some errors, we should allow the user to
> get out of these situations, after showing the error.  That is,
> instead of silently ignoring the error on some low level, we should
> propagate it to userlock.el and allow the user to decide whether
> he/she wants to ignore the error or do something about that.  These
> errors could mean something innocent, or they could mean something
> more serious, and we shouldn't second-guess which one is it.

I agree with this. However, I think there are two bugs here.

The first bug is that the low-level locking code is busted, in that it
thinks there's a lock file when ENOTDIR says there isn't. I installed
the first attached patch into master to fix this bug. This patch isn't
what Matt suggested - it's a bit earlier in the low-level C code where
the bug really occurred. (The area that Matt was looking at was later
on, when we have the lock and are removing it, and the ENOENT check
there is for the rare case where NFS is being used and its unlink
implementation gets confused and fails with ENOENT even though it was
actually successful and where any error other than ENOENT is serious.)

The second bug is that once you're in a tricky situation where the file
can't be unlocked for whatever reason and you attempt to exit Emacs,
Emacs tries to auto-save the buffer, fails because of the lock problem,
and then gets into a weird state where you cannot do anything. This
problem can happen in other scenarios. For example:

* Run Emacs and visit the file /tmp/a/b where /tmp/a does not exist.
Emacs will warn "Use M-x make-directory RET RET to create the directory
and its parents"; ignore the warning.

* Type some characters so that /tmp/a/b's buffer is nonempty.

* Create an inaccessible directory /tmp/a by running "mkdir -m 0 /tmp/a"
outside Emacs.

* Type C-x C-c to exit Emacs. It will say "Save file /tmp/a/b?" Type
"n". It will then say "Modified buffers exist; exit anyway? (yes or
no)". Type "yes". Emacs will then hang, in a weird state where it is
trying to auto-save but hanging in the middle of that.

I did not fix this latter problem, so it needs further investigation.
While looking into it, though, I did see some longstanding code in
files.el that could use a bit of sprucing up given the more-modern
primitives available now, and so installed the second attached patch
into master.

0001-Fix-file-lock-issue-Bug-46397.patch (986 bytes) Download Attachment
0002-Simplify-and-speed-up-after-find-file.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], [hidden email], Matt Armstrong <[hidden email]>
> From: Paul Eggert <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong
In reply to this post by Paul Eggert
Paul Eggert <[hidden email]> writes:

> The second bug is that once you're in a tricky situation where the file
> can't be unlocked for whatever reason and you attempt to exit Emacs,
> Emacs tries to auto-save the buffer, fails because of the lock problem,
> and then gets into a weird state where you cannot do anything. This
> problem can happen in other scenarios. For example:
>
> * Run Emacs and visit the file /tmp/a/b where /tmp/a does not exist.
> Emacs will warn "Use M-x make-directory RET RET to create the directory
> and its parents"; ignore the warning.
>
> * Type some characters so that /tmp/a/b's buffer is nonempty.
>
> * Create an inaccessible directory /tmp/a by running "mkdir -m 0 /tmp/a"
> outside Emacs.
>
> * Type C-x C-c to exit Emacs. It will say "Save file /tmp/a/b?" Type
> "n". It will then say "Modified buffers exist; exit anyway? (yes or
> no)". Type "yes". Emacs will then hang, in a weird state where it is
> trying to auto-save but hanging in the middle of that.
>
> I did not fix this latter problem, so it needs further investigation.

Paul, I looked into this second issue a bit.

The issue isn't confined to exiting Emacs. It appears that once in a
"tricky situation where the file can't be unlocked for whatever reason"
Emacs will refuse to kill the buffer because unlock_file() signals an
error.

The problem is more severe when exiting Emacs because it occurs after
Emacs is in a half shut down state. The shutdown sequence does not take
kindly to the unhandled non-local exit.

I used this for a quick repro:

(cl-flet ((command-in-tmp (prog &rest args)
                   (with-temp-buffer
                     (cd-absolute "/tmp")
                     (apply #'call-process prog nil nil nil args))))
  (command-in-tmp "chmod" "u+rwx" "a")
  (command-in-tmp "rm" "-rf" "a")
  (find-file "/tmp/a/b")
  (insert "Hello, World!")
  (command-in-tmp "mkdir" "-m" "0" "a"))

At shutdown, the following call stack signals an error:

kill_emacs
 -> shut_down_emacs
  -> unlock_all_files
   -> unlock_file
    -> report_file_errno

In this case, the errno is "permission denied."

Note that shut_down_emacs() calls Fdo_auto_save() just before
unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
report_file_errno, throwing an errno 13 (Permission denied), but that
recovers and continues.



Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
On 2/11/21 2:14 PM, Matt Armstrong wrote:

> The issue isn't confined to exiting Emacs. It appears that once in a
> "tricky situation where the file can't be unlocked for whatever reason"
> Emacs will refuse to kill the buffer because unlock_file() signals an
> error.

kill-buffer already asks the user something like "Buffer XXX modified;
kill anyway? (yes or no)" when killing a buffer that's been changed
without being saved. Perhaps it should also ask "File XXX cannot be
unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.

> Note that shut_down_emacs() calls Fdo_auto_save() just before
> unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
> report_file_errno, throwing an errno 13 (Permission denied), but that
> recovers and continues.

Presumably shut_down_emacs should recover and continue if
unlock_all_files fails when it directly calls unlock_all_files.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Paul Eggert <[hidden email]>
> Date: Thu, 11 Feb 2021 18:20:44 -0800
>
> On 2/11/21 2:14 PM, Matt Armstrong wrote:
>
> > The issue isn't confined to exiting Emacs. It appears that once in a
> > "tricky situation where the file can't be unlocked for whatever reason"
> > Emacs will refuse to kill the buffer because unlock_file() signals an
> > error.
>
> kill-buffer already asks the user something like "Buffer XXX modified;
> kill anyway? (yes or no)" when killing a buffer that's been changed
> without being saved. Perhaps it should also ask "File XXX cannot be
> unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.
>
> > Note that shut_down_emacs() calls Fdo_auto_save() just before
> > unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
> > report_file_errno, throwing an errno 13 (Permission denied), but that
> > recovers and continues.
>
> Presumably shut_down_emacs should recover and continue if
> unlock_all_files fails when it directly calls unlock_all_files.

That all is true, but I think we should provide for recovery in a way
that will work with _any_ command that calls unlock_file or
unlock_all_files.  Not just these two instances.  See my other message
about this.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Eli Zaretskii
> From: Matt Armstrong <[hidden email]>
> Cc: Paul Eggert <[hidden email]>,  [hidden email],
>   [hidden email],  [hidden email]
> Date: Wed, 10 Feb 2021 14:39:36 -0800
>
> > 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.

I said "silently" because the user is left unaware of what Emacs did
in this case.  We don't even show a warning or any other informative
message.

> 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.

"Definitely"? "safely"?  How do you arrive at that conclusion?

The Posix spec of 'unlink' says:

  [ENOTDIR]
      A component of the path prefix names an existing file that is
      neither a directory nor a symbolic link to a directory, or the
      path argument contains at least one non- <slash> character and
      ends with one or more trailing <slash> characters and the last
      pathname component names an existing file that is neither a
      directory nor a symbolic link to a directory.

It doesn't even say which component of the file name is not a
directory, nor does it distinguish between the two different use cases
that could cause ENOTDIR.  How can current_lock_owner decide, on these
shaky grounds alone, that nobody owned the lock, let alone do that
'safely"?

My point is that the values of errno provide too little information
for a safe decision here, one that couldn't possibly be wrong.  It
could be the scenario that triggered this bug report, but it could be
something entirely different.  We just don't know enough, and any
assumptions in this situation can definitely err.

Which is why I still think that we need to bring the user into the
loop.  Users will know what could or did happen, and even if they
don't, they are in charge of resolving the situation.  These problems
are rare enough to not make prompting the user for the appropriate
action an annoyance, so there's no good reason not to do so.  Doing so
will, as a nice bonus, also solve similar problems for any other value
of errno, once and for all.



Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
On 2/11/21 11:43 PM, Eli Zaretskii wrote:
>> From: Matt Armstrong <[hidden email]>

>> 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.
>
> "Definitely"? "safely"?  How do you arrive at that conclusion?
>
> The Posix spec of 'unlink' says:

Matt was talking about current_lock_owner, a function that does not call
'unlink', so it's not clear why the 'unlink' spec is relevant here.

current_lock_owner eventually calls either readlinkat or (on systems
without symlinks) openat. If either fails with ENOTDIR, some ancestor of
the lock file is a non-directory, hence the lock file itself cannot
possibly exist and therefore it must be the case that nobody owns the
lock. This is true regardless of which particular ancestor is a
non-directory. (The lockfile name does not end in "/", so we needn't
worry about that case here.)



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], [hidden email], [hidden email],
>  Matt Armstrong <[hidden email]>
> From: Paul Eggert <[hidden email]>
> Date: Fri, 12 Feb 2021 01:36:30 -0800
>
> > The Posix spec of 'unlink' says:
>
> Matt was talking about current_lock_owner, a function that does not call
> 'unlink', so it's not clear why the 'unlink' spec is relevant here.

Is the description for other APIs significantly different?  If not,
then this details is not important.

> current_lock_owner eventually calls either readlinkat or (on systems
> without symlinks) openat. If either fails with ENOTDIR, some ancestor of
> the lock file is a non-directory, hence the lock file itself cannot
> possibly exist and therefore it must be the case that nobody owns the
> lock.

Nobody owns the lock _now_, but how did that come to happen?  We have
no idea.  It could be an indication that the user's files are under
attack, for example.  So silently assuming that the lock doesn't exist
is not necessarily TRT.



Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong
Eli Zaretskii <[hidden email]> writes:

>> Cc: [hidden email], [hidden email], [hidden email],
>>  Matt Armstrong <[hidden email]>
>> From: Paul Eggert <[hidden email]>
>> Date: Fri, 12 Feb 2021 01:36:30 -0800
>>
>> > The Posix spec of 'unlink' says:
>>
>> Matt was talking about current_lock_owner, a function that does not call
>> 'unlink', so it's not clear why the 'unlink' spec is relevant here.
>
> Is the description for other APIs significantly different?  If not,
> then this details is not important.
>
>> current_lock_owner eventually calls either readlinkat or (on systems
>> without symlinks) openat. If either fails with ENOTDIR, some ancestor of
>> the lock file is a non-directory, hence the lock file itself cannot
>> possibly exist and therefore it must be the case that nobody owns the
>> lock.
>
> Nobody owns the lock _now_, but how did that come to happen?  We have
> no idea.  It could be an indication that the user's files are under
> attack, for example.  So silently assuming that the lock doesn't exist
> is not necessarily TRT.

I think this portion of the discussion is on the fringe of the important
issue. Right now, 'current_lock_owner' can return arbitrary errno
values, and if it does Emacs is in a bad situation:

 a) The user can't kill the buffer interactively.
 b) Emacs can't exit, gets in a bad state, and must be killed.

So, definitely, we need to fix that.


Separately, I continue to think Paul's recent commit is fine. With due
consideration to the issue of inferring too much about system state
based on errno inspection, ENOTDIR and ENOENT are both unambiguously
specified for the functions 'current_lock_owner' calls -- to mean the
path components that should be directories are not in fact directories
-- and so both errno values imply that the lock file isn't at the path
Emacs is has associated with the buffer.



Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> Cc: [hidden email], [hidden email]
>> From: Paul Eggert <[hidden email]>
>> Date: Thu, 11 Feb 2021 18:20:44 -0800
>>
>> On 2/11/21 2:14 PM, Matt Armstrong wrote:
>>
>> > The issue isn't confined to exiting Emacs. It appears that once in a
>> > "tricky situation where the file can't be unlocked for whatever reason"
>> > Emacs will refuse to kill the buffer because unlock_file() signals an
>> > error.
>>
>> kill-buffer already asks the user something like "Buffer XXX modified;
>> kill anyway? (yes or no)" when killing a buffer that's been changed
>> without being saved. Perhaps it should also ask "File XXX cannot be
>> unlocked; kill buffer anyway? (yes or no)" if the file can't be unlocked.
>>
>> > Note that shut_down_emacs() calls Fdo_auto_save() just before
>> > unlock_all_files() and that call succeeds. Fdo_auto_save() also calls
>> > report_file_errno, throwing an errno 13 (Permission denied), but that
>> > recovers and continues.
>>
>> Presumably shut_down_emacs should recover and continue if
>> unlock_all_files fails when it directly calls unlock_all_files.
>
> That all is true, but I think we should provide for recovery in a way
> that will work with _any_ command that calls unlock_file or
> unlock_all_files.  Not just these two instances.  See my other message
> about this.

Eli, I looked a bit at finding a general solution.

One obvious issue is my inexperience in the code base. The locking logic
all seems handled in C code, and I'm not yet very familiar with Emacs'
particularities. Most of my multi-decade long career has been in lower
level C++ code, so it is semi-familiar, but I haven't yet internalized
how to think about problems in the Emacs lispy-way. I keep grasping for
expressions of OO ideas that just aren't there, or at least aren't clear
to me. :)

One issue is that 'unlock_file' has about 10 callers (though over half
are from write-region).

I'm not sure how functions like 'write_region',
'restore-buffer-modified-p' and 'replace-buffer-contents' should be
handling lock and unlock failures.

I think save-buffers-kill-terminal should be modified to leave the
buffers in a state that won't trigger un-locking much later (after it is
too late to do proper UI interactions). If a user opts to not save a
buffer, then the unlock could happen immediately (and possibly surface
an error of its own). Sound good?

One problem with the above: currently buffers do not track whether Emacs
thinks they hold the lock. Normally I'd think about adding a "user chose
to ignore unlock errors" flag at that spot, but it doesn't exist.

Instead, code attempts to re-verify lock state from the file system at
every operation. Not even setting `create-lockfiles' to nil is enough to
prevent Emacs from attempting to delete them. Some mechanism to record
the user's desire to bypass lock file errors is needed.

There is also the case where 'kill_emacs' is called from a signal, which
seems to proceed directly to shutdown without prompting the user to save
buffers. For this flow, I think the only option is to "swallow" the
errors while unlocking files. The Emacs manually even allows or this
possibility (mentioning "if Emacs crashes..."). Sound good?

You have suggested showing a prompt. What question would it ask? I am
having trouble imagining ways of making the situation clear to the user.
There are also many situations:

 - unable to determine if Emacs has the lock
 - unable to acquire the lock
 - unable to release the lock

None of these are questions. What do we expect the user to actually
understand and answer to from these prompts? Or, is issuing a message
sufficient? A message would certainly simplify things.



Reply | Threaded
Open this post in threaded view
|

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

Paul Eggert
On 2/12/21 5:15 PM, Matt Armstrong wrote:
> You have suggested showing a prompt. What question would it ask?

How about this:

/tmp/x/y lock is invalid; assume unlocked? (yes or no)




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Matt Armstrong
> From: Matt Armstrong <[hidden email]>
> Cc: [hidden email], [hidden email]
> Date: Fri, 12 Feb 2021 15:59:40 -0800
>
> > Nobody owns the lock _now_, but how did that come to happen?  We have
> > no idea.  It could be an indication that the user's files are under
> > attack, for example.  So silently assuming that the lock doesn't exist
> > is not necessarily TRT.
>
> I think this portion of the discussion is on the fringe of the important
> issue.

For me, it _is_ the important issue.  This bug is just an indication
of a larger problem we have.

> Right now, 'current_lock_owner' can return arbitrary errno
> values, and if it does Emacs is in a bad situation:
>
>  a) The user can't kill the buffer interactively.
>  b) Emacs can't exit, gets in a bad state, and must be killed.
>
> So, definitely, we need to fix that.

So let's fix that instead of arguing, okay?

> Separately, I continue to think Paul's recent commit is fine.

I beg to differ: by solving that single aspect of a larger problem, it
makes the larger problem less urgent, and that runs high risk of
leaving it unsolved, IME.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Matt Armstrong
> From: Matt Armstrong <[hidden email]>
> Cc: [hidden email], [hidden email]
> Date: Fri, 12 Feb 2021 17:15:42 -0800
>
> One obvious issue is my inexperience in the code base. The locking logic
> all seems handled in C code, and I'm not yet very familiar with Emacs'
> particularities. Most of my multi-decade long career has been in lower
> level C++ code, so it is semi-familiar, but I haven't yet internalized
> how to think about problems in the Emacs lispy-way. I keep grasping for
> expressions of OO ideas that just aren't there, or at least aren't clear
> to me. :)

Feel free to ask questions about the internals.

> One issue is that 'unlock_file' has about 10 callers (though over half
> are from write-region).
>
> I'm not sure how functions like 'write_region',
> 'restore-buffer-modified-p' and 'replace-buffer-contents' should be
> handling lock and unlock failures.

Why not in a unified manner?  IOW, define a single function to handle
the situation where unlocking triggers some error from lower-level
APIs such as 'unlink', and call it from unlock_file no matter what was
its caller?

> I think save-buffers-kill-terminal should be modified to leave the
> buffers in a state that won't trigger un-locking much later (after it is
> too late to do proper UI interactions). If a user opts to not save a
> buffer, then the unlock could happen immediately (and possibly surface
> an error of its own). Sound good?

I don't think I understand what you are proposing here in enough
detail.  You seem to be describing several issues together, and I
don't think I understand how you propose to solve each one of them.

> One problem with the above: currently buffers do not track whether Emacs
> thinks they hold the lock. Normally I'd think about adding a "user chose
> to ignore unlock errors" flag at that spot, but it doesn't exist.

Why would we need such a flag?  Can you show a couple of use cases
where it would be necessary?

> Instead, code attempts to re-verify lock state from the file system at
> every operation. Not even setting `create-lockfiles' to nil is enough to
> prevent Emacs from attempting to delete them. Some mechanism to record
> the user's desire to bypass lock file errors is needed.

Why is such an option needed?  If we can reasonably deal with failures
to unlock each file, wouldn't that be enough?

> There is also the case where 'kill_emacs' is called from a signal, which
> seems to proceed directly to shutdown without prompting the user to save
> buffers. For this flow, I think the only option is to "swallow" the
> errors while unlocking files. The Emacs manually even allows or this
> possibility (mentioning "if Emacs crashes..."). Sound good?

But we do ask about auto-save errors in these cases, don't we?  If so,
why not ask about locks as well?

Thanks for working on this.



Reply | Threaded
Open this post in threaded view
|

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

Matt Armstrong
Eli Zaretskii <[hidden email]> writes:

>> There is also the case where 'kill_emacs' is called from a signal,
>> which
[...]
> But we do ask about auto-save errors in these cases, don't we?  If so,
> why not ask about locks as well?

I think auto-save issues messages and warnings, but never prompts ("asks
about").

Auto save can call `display-warning' (see auto_save_error in fileio.c)
or `message' (see message1() and message_with_string() calls in same). I
can't find any prompts.

So let's settle a question before going further. Would you be okay if
lock errors were reported in the same way(s) auto-save errors are?

I should reveal: during shut down, both `message' and `display-warning'
seems to have no effect. The first thing `shut_down_emacs()' does is
inhibit display, which is probably why.

Taking this approach would be a tremendous simplification over what I
had been thiking you were asking for. Most of my confusion centers
around re-aranging code flow such that all possible errors occur, by
construction, only *before* shut_down_emacs() may be called.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Matt Armstrong <[hidden email]>
> Cc: [hidden email], [hidden email], [hidden email]
> Date: Sat, 13 Feb 2021 16:49:43 -0800
>
> I think auto-save issues messages and warnings, but never prompts ("asks
> about").
>
> Auto save can call `display-warning' (see auto_save_error in fileio.c)
> or `message' (see message1() and message_with_string() calls in same). I
> can't find any prompts.
>
> So let's settle a question before going further. Would you be okay if
> lock errors were reported in the same way(s) auto-save errors are?

I'm okay with doing the same as auto-save does when Emacs is shutting
down, yes.  As for other situations, I'm not sure we should just
display a warning.

Were you asking only about what to do during shutdown?



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Matt Armstrong <[hidden email]>
> Cc: [hidden email], [hidden email], [hidden email]
> Date: Sun, 14 Feb 2021 14:16:12 -0800
>
> When releasing the lock, I have a less clear opinion but I'm thinking
> that warnings are better. A warning is still quite intrusive and
> obvious. Maybe we don't need to decide this part now.

The problem with warnings is that they can go unnoticed, unless
followed by sit-for.



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Matt Armstrong <[hidden email]> writes:

> My FSF papers are out of date. Mind sending me new ones?  I am in the
> United States.

Here's the form to get started:


Please email the following information to [hidden email], and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]




12