bug#964: trash - runaway recursion for cross-device delete-file ops

bug#964: trash - runaway recursion for cross-device delete-file ops

[David De La Harpe Golden]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

Package: emacs
Version: 23.0.60
Severity: normal
Tags: patch

Steps to reproduce:

Turn on delete-by-moving-to-trash on gnu+linux.
Make a file on a different filesystem to your home dir.
Try to delete-file the file in emacs once.
Look inside ~/.Trash, full of hundreds of redundant
copies of the file.

Reason:

When delete-by-moving-to-trash is on, delete-file calls move-file-to-trash.

move-file-to-trash, when using its fallback emacs trashcan
implementation* (i.e. when there's no system-move-file-to-trash) picks a
name for the trashed file, and calls rename-file.

rename-file tries a C rename(), but that doesn't work cross-device on
gnu+linux (and several other OSes), so rename-file falls back to copying
the file to the new name, and then calls delete-file to get rid of the
old one (~line 2247 of fileio.c)

move-file-to-trash decides the existing filename under .Trash is taken,
seeing as a file exists with that name, picks a new name, and calls
rename-file again...

... Lather, rinse, repeat, until emacs gets bored with a
"Variable binding depth exceeds max-specpdl-size"

Fix?:

(i) Well, binding delete-by-moving-to-trash to nil around the
rename-file in move-file-to-trash might be adequate, fixes immediate
issue?  trivial patch attached.  Though another strikes me:

(ii) As it stands, won't an actual rename-file will sometimes move a
copy of "renamed" files to trash (i.e. when they're on a different
device) if delete-by-moving-to-trash is on?  Not sure that's very nice -
maybe should bind delete-by-moving-to-trash to nil around its call to
delete-file, or maybe just use C unlink()?


* which really isn't suitable for gnu+linux/freedesktop.org desktops,
it seems to be something very like (if not identical to) the macosx
convention, but that's a matter for a different bug.







[-- Attachment #2: trash_xdevice_fix_r1.diff --]
[-- Type: text/x-patch, Size: 1141 bytes --]

Index: lisp/files.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/files.el,v
retrieving revision 1.995
diff -U 8 -r1.995 files.el
--- lisp/files.el	2 Sep 2008 16:10:44 -0000	1.995
+++ lisp/files.el	12 Sep 2008 01:43:51 -0000
@@ -5820,17 +5820,18 @@
            ;; make new-fn unique.
            ;; example: "~/.Trash/abc.txt" -> "~/.Trash/abc.txt.~1~"
            (let ((version-control t))
              (setq new-fn (car (find-backup-file-name new-fn)))))
       ;; stop processing if fn is same or parent directory of trash-dir.
       (and (string-match fn trash-dir)
            (error "Filename `%s' is same or parent directory of trash-directory"
                   filename))
-      (rename-file fn new-fn)))))
+      (let ((delete-by-moving-to-trash nil))
+	(rename-file fn new-fn))))))
 
 \f
 (define-key ctl-x-map "\C-f" 'find-file)
 (define-key ctl-x-map "\C-r" 'find-file-read-only)
 (define-key ctl-x-map "\C-v" 'find-alternate-file)
 (define-key ctl-x-map "\C-s" 'save-buffer)
 (define-key ctl-x-map "s" 'save-some-buffers)
 (define-key ctl-x-map "\C-w" 'write-file)

[-- Attachment #3: ChangeLog.trash_xdevice_fix_r1 --]
[-- Type: text/plain, Size: 219 bytes --]

2008-09-11  David De La Harpe Golden <david@harpegolden.net>

	files.el: in move-file-to-trash, bind delete-by-moving-to-trash to
	nil around rename-file to avoid recursive trashing if rename-file
	calls delete-file.

bug#964: marked as done (trash - runaway recursion for cross-device delete-file ops)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]


Your message dated Sat, 20 Sep 2008 17:40:39 -0400
with message-id <n8fxnufqco.fsf@fencepost.gnu.org>
and subject line Re: bug#964: trash - runaway recursion for cross-device delete-file ops
has caused the Emacs bug report #964,
regarding trash - runaway recursion for cross-device delete-file ops
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
964: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=964
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 5176 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 1810 bytes --]

Package: emacs
Version: 23.0.60
Severity: normal
Tags: patch

Steps to reproduce:

Turn on delete-by-moving-to-trash on gnu+linux.
Make a file on a different filesystem to your home dir.
Try to delete-file the file in emacs once.
Look inside ~/.Trash, full of hundreds of redundant
copies of the file.

Reason:

When delete-by-moving-to-trash is on, delete-file calls move-file-to-trash.

move-file-to-trash, when using its fallback emacs trashcan
implementation* (i.e. when there's no system-move-file-to-trash) picks a
name for the trashed file, and calls rename-file.

rename-file tries a C rename(), but that doesn't work cross-device on
gnu+linux (and several other OSes), so rename-file falls back to copying
the file to the new name, and then calls delete-file to get rid of the
old one (~line 2247 of fileio.c)

move-file-to-trash decides the existing filename under .Trash is taken,
seeing as a file exists with that name, picks a new name, and calls
rename-file again...

... Lather, rinse, repeat, until emacs gets bored with a
"Variable binding depth exceeds max-specpdl-size"

Fix?:

(i) Well, binding delete-by-moving-to-trash to nil around the
rename-file in move-file-to-trash might be adequate, fixes immediate
issue?  trivial patch attached.  Though another strikes me:

(ii) As it stands, won't an actual rename-file will sometimes move a
copy of "renamed" files to trash (i.e. when they're on a different
device) if delete-by-moving-to-trash is on?  Not sure that's very nice -
maybe should bind delete-by-moving-to-trash to nil around its call to
delete-file, or maybe just use C unlink()?


* which really isn't suitable for gnu+linux/freedesktop.org desktops,
it seems to be something very like (if not identical to) the macosx
convention, but that's a matter for a different bug.







[-- Attachment #2.1.2: trash_xdevice_fix_r1.diff --]
[-- Type: text/x-patch, Size: 1141 bytes --]

Index: lisp/files.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/files.el,v
retrieving revision 1.995
diff -U 8 -r1.995 files.el
--- lisp/files.el	2 Sep 2008 16:10:44 -0000	1.995
+++ lisp/files.el	12 Sep 2008 01:43:51 -0000
@@ -5820,17 +5820,18 @@
            ;; make new-fn unique.
            ;; example: "~/.Trash/abc.txt" -> "~/.Trash/abc.txt.~1~"
            (let ((version-control t))
              (setq new-fn (car (find-backup-file-name new-fn)))))
       ;; stop processing if fn is same or parent directory of trash-dir.
       (and (string-match fn trash-dir)
            (error "Filename `%s' is same or parent directory of trash-directory"
                   filename))
-      (rename-file fn new-fn)))))
+      (let ((delete-by-moving-to-trash nil))
+	(rename-file fn new-fn))))))
 
 \f
 (define-key ctl-x-map "\C-f" 'find-file)
 (define-key ctl-x-map "\C-r" 'find-file-read-only)
 (define-key ctl-x-map "\C-v" 'find-alternate-file)
 (define-key ctl-x-map "\C-s" 'save-buffer)
 (define-key ctl-x-map "s" 'save-some-buffers)
 (define-key ctl-x-map "\C-w" 'write-file)

[-- Attachment #2.1.3: ChangeLog.trash_xdevice_fix_r1 --]
[-- Type: text/plain, Size: 219 bytes --]

2008-09-11  David De La Harpe Golden <david@harpegolden.net>

	files.el: in move-file-to-trash, bind delete-by-moving-to-trash to
	nil around rename-file to avoid recursive trashing if rename-file
	calls delete-file.



[-- Attachment #3: Type: message/rfc822, Size: 2217 bytes --]

From: Glenn Morris <rgm@gnu.org>
To: David De La Harpe Golden <david@harpegolden.net>
Cc: 964-done@emacsbugs.donarmstrong.com
Subject: Re: bug#964: trash - runaway recursion for cross-device delete-file ops
Date: Sat, 20 Sep 2008 17:40:39 -0400
Message-ID: <n8fxnufqco.fsf@fencepost.gnu.org>

David De La Harpe Golden wrote:

> (i) Well, binding delete-by-moving-to-trash to nil around the
> rename-file in move-file-to-trash might be adequate, fixes immediate
> issue?  trivial patch attached.

Thanks; applied.

> (ii) As it stands, won't an actual rename-file will sometimes move a
> copy of "renamed" files to trash (i.e. when they're on a different
> device) if delete-by-moving-to-trash is on?  Not sure that's very nice -
> maybe should bind delete-by-moving-to-trash to nil around its call to
> delete-file, or maybe just use C unlink()?

I installed a fix using the former approach.

This feature doesn't seem very well thought-out on non-Windows platforms.