all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
@ 2013-12-13 19:51 Artem Chuprina
  2013-12-13 22:51 ` Glenn Morris
                   ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Artem Chuprina @ 2013-12-13 19:51 UTC (permalink / raw)
  To: 16133

Please describe exactly what actions triggered the bug, and
the precise symptoms of the bug.  If you can, give a recipe
starting from `emacs -Q':

Having org on ext4 filesystem with 0664 permission on org/work.org, and
c1 on FAT filesystem with file permissions 0075 (in fact, SD card on
Android system, so such strange permissions) I do

M-: copy-file("org/work.org" "c1/work.org" t)

and get an error

Debugger entered--Lisp error: (file-error "Doing chmod" "operation not
permitted" $
  copy-file("org/work.org" "c1/work.org" t)
  eval((copy-file "org/work.org" "c1/work.org" t) nil)
  eval-expression((copy-file "org/work.org" "c1/work.org" t) nil)
  call-interactively(eval-expression nil nil)

That's right, it cannot do a chmod, but it cannot do it there at all, so
there is no sense to fail here.

The problem does not exist in GNU Emacs 23.4.1
(arm-unknown-linux-gnueabihf) of 2013-07-01 on henze, modified by Debian

In GNU Emacs 24.3.1 (arm-unknown-linux-gnueabihf)
 of 2013-10-01 on hummel, modified by Debian
Configured using:
 `configure '--build' 'arm-linux-gnueabihf' '--build'
 'arm-linux-gnueabihf' '--prefix=/usr' '--sharedstatedir=/var/lib'
 '--libexecdir=/usr/lib' '--localstatedir=/var/lib'
 '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes'

'--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.3/site-l\
isp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.3/site-lisp:/usr/share/em\
acs/site-lisp'
 '--with-crt-dir=/usr/lib/arm-linux-gnueabihf' '--with-x=no'
 '--without-gconf' '--without-gsettings'
 'build_alias=arm-linux-gnueabihf' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'LDFLAGS=-Wl,-z,relro' 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LC_MESSAGES: C
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Fundamental

Recent input:
ESC : ( c o p y - f i l e
SPC " o r g / e m a c s . DEL DEL DEL DEL DEL DEL w
o r k . o r g " SPC " c 1 / w o r k . o r g " ) RET
C-]





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-13 19:51 bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem Artem Chuprina
@ 2013-12-13 22:51 ` Glenn Morris
  2013-12-13 22:55   ` Glenn Morris
  2013-12-14 10:10   ` Artem Chuprina
  2013-12-20 23:27 ` Paul Eggert
  2013-12-22  0:01 ` Paul Eggert
  2 siblings, 2 replies; 33+ messages in thread
From: Glenn Morris @ 2013-12-13 22:51 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

Artem Chuprina wrote:

> Having org on ext4 filesystem with 0664 permission on org/work.org, and
> c1 on FAT filesystem with file permissions 0075 (in fact, SD card on
> Android system, so such strange permissions) I do
>
> M-: copy-file("org/work.org" "c1/work.org" t)
>
> and get an error
>
> Debugger entered--Lisp error: (file-error "Doing chmod" "operation not
> permitted" $

I'm not sure this is a bug.
The file is copied, but there is an error trying to preserve permissions.
Because eval-expression-debug-on-error is non-nil by default, the
debugger pops up.

Emacs's behaviour seems the same as using

cp -a org/work.org c1/work.org

on the command-line. "-a" is appropriate because C-h f copy-file says:

  This function always sets the file modes of the output file to match
  the input file.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-13 22:51 ` Glenn Morris
@ 2013-12-13 22:55   ` Glenn Morris
  2013-12-14 10:10   ` Artem Chuprina
  1 sibling, 0 replies; 33+ messages in thread
From: Glenn Morris @ 2013-12-13 22:55 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

Glenn Morris wrote:

> Emacs's behaviour seems the same as using
>
> cp -a org/work.org c1/work.org
>
> on the command-line.

"cp --preserve=mode" would have been a better analogy.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-13 22:51 ` Glenn Morris
  2013-12-13 22:55   ` Glenn Morris
@ 2013-12-14 10:10   ` Artem Chuprina
  2013-12-14 20:19     ` Glenn Morris
  1 sibling, 1 reply; 33+ messages in thread
From: Artem Chuprina @ 2013-12-14 10:10 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 16133

Glenn Morris -> Artem Chuprina  @ Fri, 13 Dec 2013 17:51:34 -0500:

 >> Having org on ext4 filesystem with 0664 permission on org/work.org, and
 >> c1 on FAT filesystem with file permissions 0075 (in fact, SD card on
 >> Android system, so such strange permissions) I do
 >>
 >> M-: copy-file("org/work.org" "c1/work.org" t)
 >>
 >> and get an error
 >>
 >> Debugger entered--Lisp error: (file-error "Doing chmod" "operation not
 >> permitted" $

 GM> I'm not sure this is a bug.
 GM> The file is copied, but there is an error trying to preserve permissions.
 GM> Because eval-expression-debug-on-error is non-nil by default, the
 GM> debugger pops up.

 GM> Emacs's behaviour seems the same as using

 GM> cp -a org/work.org c1/work.org

 GM> on the command-line. "-a" is appropriate because C-h f copy-file says:

 GM>   This function always sets the file modes of the output file to match
 GM>   the input file.

The problem is that now it is impossible to use other functions that use
copy-file.  I've encountered this bug trying to use org-mobile-push
that, among other, copies several files to its target directory (which I
want to be on FAT partition for Android's MobileOrg).  On the first such
file, copy-file throws an error, and whole operation stops there and so
fails.  org-mobile-push catches error and don't enter a debugger, but it
don't work either, thinking that it failed to copy file.  Yet file is
indeed copied, just (totally unneeded in this case) chmod failed.

I think that it is a good idea to _attempt to_ copy permissions.  But it
is a bad idea to throw an error if file is copied well but we cannot set
permissions.  These days it is very common to copy files between
incompatible file systems.  Most of us use FAT, many use CIFS, there are
also NFS, davfs, sshfs etc.  So I think that a reasonable behavior would
be to try to set permissions, but don't fail on chmod error.  At least,
on copy-file level (if chmod function itself throws an error, this is
ok, but copy-file should not).





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 10:10   ` Artem Chuprina
@ 2013-12-14 20:19     ` Glenn Morris
  2013-12-14 20:46       ` Josh
                         ` (3 more replies)
  0 siblings, 4 replies; 33+ messages in thread
From: Glenn Morris @ 2013-12-14 20:19 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: Paul Eggert, 16133


I think you are right that it is a problem.
I think the problem was in documenting copy-file to always copy
permissions in the first place. But I imagine it is far too late to
change that. Not sure what the right thing to do is here...

Someone may be relying on the fact that copy-file copies permissions,
and want it it throw an error if it fails:

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8306

Others, like you, may not care.

Since copy-file has only relatively recently starting giving an error,
maybe the least bad thing would be for it to give a message instead of
an error?

I don't know how we tidy this up for the future though.
Add yet another optional argument "error-if-permissions-fail"? Yuck.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:19     ` Glenn Morris
@ 2013-12-14 20:46       ` Josh
  2013-12-14 20:57         ` Eli Zaretskii
  2013-12-14 20:55       ` Eli Zaretskii
                         ` (2 subsequent siblings)
  3 siblings, 1 reply; 33+ messages in thread
From: Josh @ 2013-12-14 20:46 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Paul Eggert, Artem Chuprina, 16133

On Sat, Dec 14, 2013 at 12:19 PM, Glenn Morris <rgm@gnu.org> wrote:
> I don't know how we tidy this up for the future though.
> Add yet another optional argument "error-if-permissions-fail"? Yuck.

It might be reasonable to error only when the existing
PRESERVE-UID-GID argument was non-nil.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:19     ` Glenn Morris
  2013-12-14 20:46       ` Josh
@ 2013-12-14 20:55       ` Eli Zaretskii
  2013-12-14 21:07       ` Achim Gratz
  2013-12-15 14:38       ` Artem Chuprina
  3 siblings, 0 replies; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-14 20:55 UTC (permalink / raw)
  To: Glenn Morris; +Cc: eggert, ran, 16133

> From: Glenn Morris <rgm@gnu.org>
> Date: Sat, 14 Dec 2013 15:19:33 -0500
> Cc: Paul Eggert <eggert@cs.ucla.edu>, 16133@debbugs.gnu.org
> 
> Since copy-file has only relatively recently starting giving an error,
> maybe the least bad thing would be for it to give a message instead of
> an error?

Yes, I think this is the way to go.  That's what "cp -p" does, FWIW.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:46       ` Josh
@ 2013-12-14 20:57         ` Eli Zaretskii
  2013-12-14 21:21           ` Josh
  0 siblings, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-14 20:57 UTC (permalink / raw)
  To: Josh; +Cc: ran, 16133, eggert

> From: Josh <josh@foxtail.org>
> Date: Sat, 14 Dec 2013 12:46:59 -0800
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Artem Chuprina <ran@lasgalen.net>,
> 	16133@debbugs.gnu.org
> 
> On Sat, Dec 14, 2013 at 12:19 PM, Glenn Morris <rgm@gnu.org> wrote:
> > I don't know how we tidy this up for the future though.
> > Add yet another optional argument "error-if-permissions-fail"? Yuck.
> 
> It might be reasonable to error only when the existing
> PRESERVE-UID-GID argument was non-nil.

That argument has nothing to do with mode bits, which is what this bug
is about.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:19     ` Glenn Morris
  2013-12-14 20:46       ` Josh
  2013-12-14 20:55       ` Eli Zaretskii
@ 2013-12-14 21:07       ` Achim Gratz
  2013-12-15 14:38       ` Artem Chuprina
  3 siblings, 0 replies; 33+ messages in thread
From: Achim Gratz @ 2013-12-14 21:07 UTC (permalink / raw)
  To: 16133

Glenn Morris writes:
> Someone may be relying on the fact that copy-file copies permissions,
> and want it it throw an error if it fails:
>
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8306
>
> Others, like you, may not care.

If the contract of copy-file is that it always copies permissions, then
it should throw an error and not silently corrupt the permissions.  That
makes it useless on filesystems without permissions and where/when the
user has insufficient rights to install permissions which is possible
when ACL are in effect, so the function would need to have an extra
argument to ignore the error or not try to change permissions at all.

> Since copy-file has only relatively recently starting giving an error,
> maybe the least bad thing would be for it to give a message instead of
> an error?

In this case an alternative would be that copying permissions is done on
a best effort basis and may fail across file systems or in certain other
situations without raising an error (although I still think that it
should be possible to check for that).

> I don't know how we tidy this up for the future though.
> Add yet another optional argument "error-if-permissions-fail"? Yuck.

How about copy-file-with-permissions (which fails when it can't do what
the name says) and copy-file which does best-effort?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada






^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:57         ` Eli Zaretskii
@ 2013-12-14 21:21           ` Josh
  2013-12-15  3:44             ` Eli Zaretskii
  0 siblings, 1 reply; 33+ messages in thread
From: Josh @ 2013-12-14 21:21 UTC (permalink / raw)
  To: Eli Zaretskii
  Cc: Валерия Лихушина,
	16133, Paul Eggert

On Sat, Dec 14, 2013 at 12:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Josh <josh@foxtail.org>
>> Date: Sat, 14 Dec 2013 12:46:59 -0800
>> Cc: Paul Eggert <eggert@cs.ucla.edu>, Artem Chuprina <ran@lasgalen.net>,
>>       16133@debbugs.gnu.org
>>
>> On Sat, Dec 14, 2013 at 12:19 PM, Glenn Morris <rgm@gnu.org> wrote:
>> > I don't know how we tidy this up for the future though.
>> > Add yet another optional argument "error-if-permissions-fail"? Yuck.
>>
>> It might be reasonable to error only when the existing
>> PRESERVE-UID-GID argument was non-nil.
>
> That argument has nothing to do with mode bits, which is what this bug
> is about.

That argument being nil indicates that the user doesn't care about
preserving UID and GID.  Are there cases when the user would care
about preserving mode bits but not UID and GID?





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 21:21           ` Josh
@ 2013-12-15  3:44             ` Eli Zaretskii
  0 siblings, 0 replies; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-15  3:44 UTC (permalink / raw)
  To: Josh; +Cc: ran, 16133, eggert

> From: Josh <josh@foxtail.org>
> Date: Sat, 14 Dec 2013 13:21:58 -0800
> Cc: Glenn Morris <rgm@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>, 
> 	Валерия Лихушина <ran@lasgalen.net>, 
> 	16133@debbugs.gnu.org
> 
> >> It might be reasonable to error only when the existing
> >> PRESERVE-UID-GID argument was non-nil.
> >
> > That argument has nothing to do with mode bits, which is what this bug
> > is about.
> 
> That argument being nil indicates that the user doesn't care about
> preserving UID and GID.  Are there cases when the user would care
> about preserving mode bits but not UID and GID?

Definitely.  MS-Windows is one such case.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-14 20:19     ` Glenn Morris
                         ` (2 preceding siblings ...)
  2013-12-14 21:07       ` Achim Gratz
@ 2013-12-15 14:38       ` Artem Chuprina
  2013-12-16 14:15         ` Stefan Monnier
  3 siblings, 1 reply; 33+ messages in thread
From: Artem Chuprina @ 2013-12-15 14:38 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Paul Eggert, 16133

Glenn Morris -> Artem Chuprina  @ Sat, 14 Dec 2013 15:19:33 -0500:

 GM> I think you are right that it is a problem.
 GM> I think the problem was in documenting copy-file to always copy
 GM> permissions in the first place. But I imagine it is far too late to
 GM> change that. Not sure what the right thing to do is here...

 GM> Someone may be relying on the fact that copy-file copies permissions,
 GM> and want it it throw an error if it fails:

 GM> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8306

 GM> Others, like you, may not care.

To clarify, many others, including me, CARE of copy-file NOT throwing
error when it can copy file, but not its permissions, as it did for many
years before.  That is, the mentioned patch broke that for-many-years
successful behavior, and me personally had to return back to emacs 23 to
avoid it.

Also, it seems that backup facility also depends on copy-file (probably
not, I've not done a research, but I've seen message "cannot make a
backup file, backing up to ~/.emacs.d/%backup" (or like this) when
editing file on the same VFAT filesystem.

 GM> Since copy-file has only relatively recently starting giving an error,
 GM> maybe the least bad thing would be for it to give a message instead of
 GM> an error?

As for me, this would be OK, because I cannot imagine any scenario when
I'd prefer copied file to be left at target, but copy-file throwing an
error.  I want either success if file contents was copied successfully
(and possibly a message informing me that chmod or chown failed), or, if
I REALLY CARE OF EXACT COPY, that file itself is NOT copied at all (that
is, reverted if ok-if-exist was true).

Yes, I think that chown or copying ACLs also should not throw an error
on failure.

 GM> I don't know how we tidy this up for the future though.
 GM> Add yet another optional argument "error-if-permissions-fail"? Yuck.

I think no one will end up with that argument set true.  Probably,
someone will try to, but his users will soon ask him not to do so.

Another possible solution would be to introduce a global variable, say,
copy-file-signal-error-on-content-only :), allowing end-user (that is,
not a library author who calls copy-file, like org-mobile-push in my
case, but end user, like me) to disable throwing errors if file content
copied ok, but meta information copying failed (not only chmod, but also
chown, copying ACLs, times etc.)  I expect that most users would like
that variable to be true by default :)





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-15 14:38       ` Artem Chuprina
@ 2013-12-16 14:15         ` Stefan Monnier
  0 siblings, 0 replies; 33+ messages in thread
From: Stefan Monnier @ 2013-12-16 14:15 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: Paul Eggert, 16133

GM> Since copy-file has only relatively recently starting giving an error,
GM> maybe the least bad thing would be for it to give a message instead of
GM> an error?

Yes, that's the better choice for now.


        Stefan





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-13 19:51 bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem Artem Chuprina
  2013-12-13 22:51 ` Glenn Morris
@ 2013-12-20 23:27 ` Paul Eggert
  2013-12-22  0:01 ` Paul Eggert
  2 siblings, 0 replies; 33+ messages in thread
From: Paul Eggert @ 2013-12-20 23:27 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

I couldn't reproduce the problem on my Ubuntu system
(Ubuntu 13.10); when I copied a file to a FAT file system
there was no diagnostic.

I don't understand what is meant by
"FAT filesystem with file permissions 0075".
I tried to create a directory with those permissions,
using "mkdir dir; chmod 0075 dir", but as far as Ubuntu
was concerned the chmod had no effect (it claimed to be
successful, but it didn't change the directory permissions).
Similarly, when Emacs copy-file runs, chmod has no effect
but returns 0 so Emacs thinks the chmod worked and does
not throw an error.

Perhaps there's something unusual about how your file system
was mounted?  If so, it'd help to know more about it in
order to reproduce the problem.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-13 19:51 bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem Artem Chuprina
  2013-12-13 22:51 ` Glenn Morris
  2013-12-20 23:27 ` Paul Eggert
@ 2013-12-22  0:01 ` Paul Eggert
  2013-12-22  3:47   ` Eli Zaretskii
  2 siblings, 1 reply; 33+ messages in thread
From: Paul Eggert @ 2013-12-22  0:01 UTC (permalink / raw)
  To: 16133; +Cc: Artem Chuprina

> That's what "cp -p" does, FWIW.

I just checked the source code, and it looks like 'cp -p'
reports an error if fchmod fails, so trunk Emacs
is behaving similarly to GNU cp here.

Artem, can you run Emacs under strace, and give the
exact sequence of system calls just before
Emacs reports an error?  That may help us
reproduce the problem.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22  0:01 ` Paul Eggert
@ 2013-12-22  3:47   ` Eli Zaretskii
  2013-12-22  4:01     ` Paul Eggert
  0 siblings, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-22  3:47 UTC (permalink / raw)
  To: Paul Eggert; +Cc: ran, 16133

> Date: Sat, 21 Dec 2013 16:01:07 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Artem Chuprina <ran@lasgalen.net>, Eli Zaretskii <eliz@gnu.org>
> 
> > That's what "cp -p" does, FWIW.
> 
> I just checked the source code, and it looks like 'cp -p'
> reports an error if fchmod fails

It reports an error, but keeps copying the rest of the files.

> so trunk Emacs is behaving similarly to GNU cp here.

No, it doesn't: Emacs throws an error, and stops the copy operation,
even if there are more files to copy.

The suggestion was to report an error, but not throw to top level.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22  3:47   ` Eli Zaretskii
@ 2013-12-22  4:01     ` Paul Eggert
  2013-12-22 15:50       ` Artem Chuprina
  2013-12-22 16:24       ` Eli Zaretskii
  0 siblings, 2 replies; 33+ messages in thread
From: Paul Eggert @ 2013-12-22  4:01 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ran, 16133

Eli Zaretskii wrote:
> Emacs throws an error, and stops the copy operation,
> even if there are more files to copy.

copy-file copies just one file, so there can't be any more
files to copy.  When GNU 'cp' is acting like copy-file and
is copying just one file, it doesn't do anything more to the
file after fchmod fails -- it simply exits with nonzero status,
which corresponds to Emacs throwing an error.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22  4:01     ` Paul Eggert
@ 2013-12-22 15:50       ` Artem Chuprina
  2013-12-22 19:03         ` Paul Eggert
  2013-12-22 16:24       ` Eli Zaretskii
  1 sibling, 1 reply; 33+ messages in thread
From: Artem Chuprina @ 2013-12-22 15:50 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 16133

Paul Eggert -> Eli Zaretskii  @ Sat, 21 Dec 2013 20:01:24 -0800:

 >> Emacs throws an error, and stops the copy operation,
 >> even if there are more files to copy.

 PE> copy-file copies just one file, so there can't be any more
 PE> files to copy.  When GNU 'cp' is acting like copy-file and
 PE> is copying just one file, it doesn't do anything more to the
 PE> file after fchmod fails -- it simply exits with nonzero status,
 PE> which corresponds to Emacs throwing an error.

Please note that, first, GNU cp HAS mode that allows to use in scripts
when copying to other filesystems.  That is, it has a mode in which it
returns success if it successfully copied file, but not its meta
information.  copy-file in emacs 24 - HAS NOT.  Moreover, GNU cp works
just in that mode BY DEFAULT.  That is, its authors do understand that
this is the mode of operation that most users want, not the mode that
fails in that situation.

So, if I write

#!/bin/sh -e
cp /from/ext2/filesystem/... /to/fat/filesystem/
# some code follows that depend on files on target

I get an error from cp ONLY if it could not copy file(s) (this is quite
reasonable, and this is just what most cp users want), not if it could
not copy permissions (this has no sense in this case at all, FAT
filesystem just has no permissions, and chmod always fails there).  If
for some strange reason I really insist on copying permissions also, I
should explicitly state it, and in this case this would be my fault, not
cp authors.

With copy-file in emacs24 I just cannot write such code, and moreover,
much code that used copy-file before, just stopped working.  (I've
mentioned org-mobile-push, and emacs' own file backup code, just to note
two).

Now ALL those libraries authors, including emacs authors, need to wrap
EVERY call to copy-file to catch code, and thoroughly filter exceptions
to check whether this was just chmod failure (which is acceptable) or
real copy failure.  Because in reality only copy problem is not
acceptable, not chmod problem.  This is the problem with copy-file.

And I don't remember exactly, but it seems to me that even defadvice is
not guaranteed to help, because copy-file is a primitive function, and
can be called from C code - I really suspect that backup problem cannot
be fixed by advice just by this reason.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22  4:01     ` Paul Eggert
  2013-12-22 15:50       ` Artem Chuprina
@ 2013-12-22 16:24       ` Eli Zaretskii
  2013-12-22 17:37         ` Paul Eggert
  1 sibling, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-22 16:24 UTC (permalink / raw)
  To: Paul Eggert; +Cc: ran, 16133

> Date: Sat, 21 Dec 2013 20:01:24 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 16133@debbugs.gnu.org, ran@lasgalen.net
> 
> Eli Zaretskii wrote:
> > Emacs throws an error, and stops the copy operation,
> > even if there are more files to copy.
> 
> copy-file copies just one file, so there can't be any more
> files to copy.  When GNU 'cp' is acting like copy-file and
> is copying just one file, it doesn't do anything more to the
> file after fchmod fails -- it simply exits with nonzero status,
> which corresponds to Emacs throwing an error.

I was referring to the use case described here:

  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16133#14

The 'cp' equivalent of that is when it is invoked to copy more than
one file, or copy a directory.  In that case, it does not exit on the
first fchown error (at least the version  I have here doesn't), but
instead reports the error and continues.

Signaling an error from copy-file makes it inconvenient to write
functions that copy many files or entire directories.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 16:24       ` Eli Zaretskii
@ 2013-12-22 17:37         ` Paul Eggert
  2013-12-22 18:35           ` Eli Zaretskii
  0 siblings, 1 reply; 33+ messages in thread
From: Paul Eggert @ 2013-12-22 17:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ran, 16133

Eli Zaretskii wrote:
> I was referring to the use case described here:
> 
>   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16133#14
> 
> The 'cp' equivalent of that is when it is invoked to copy more than
> one file, or copy a directory.

The use case described there does not involve merely copy-file;
it also involves some other function (org-mobile-push, I guess)
that uses copy-file to copy files one at a time,
and which gives up if one instance of copy-file fails.

This is akin to a shell script that uses cp to
copy files one a time, and which aborts if cp fails.
Such a script would behave the same way that org-mobile-push
behaves now, if I'm understanding the bug report correctly.

One fix would be to modify org-mobile-push to not throw an error
when copy-file fails, but instead to continue.  Perhaps this is the
desired behavior, not only for fchmod failure, but for other copy-file
failures such as write error.  In any case org-mobile-push could 
ignore some or all errors, or report them at the end, or whatever
it likes.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 17:37         ` Paul Eggert
@ 2013-12-22 18:35           ` Eli Zaretskii
  2013-12-22 18:54             ` Paul Eggert
  0 siblings, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-22 18:35 UTC (permalink / raw)
  To: Paul Eggert; +Cc: ran, 16133

> Date: Sun, 22 Dec 2013 09:37:26 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 16133@debbugs.gnu.org, ran@lasgalen.net
> 
> Eli Zaretskii wrote:
> > I was referring to the use case described here:
> > 
> >   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16133#14
> > 
> > The 'cp' equivalent of that is when it is invoked to copy more than
> > one file, or copy a directory.
> 
> The use case described there does not involve merely copy-file;
> it also involves some other function (org-mobile-push, I guess)
> that uses copy-file to copy files one at a time,
> and which gives up if one instance of copy-file fails.
> 
> This is akin to a shell script that uses cp to
> copy files one a time, and which aborts if cp fails.
> Such a script would behave the same way that org-mobile-push
> behaves now, if I'm understanding the bug report correctly.
> 
> One fix would be to modify org-mobile-push to not throw an error
> when copy-file fails, but instead to continue.  Perhaps this is the
> desired behavior, not only for fchmod failure, but for other copy-file
> failures such as write error.  In any case org-mobile-push could 
> ignore some or all errors, or report them at the end, or whatever
> it likes.

Sorry, I disagree: it is not the business of copy-file to decide when
to interrupt its caller.  It is only justified to do that when the
error is fatal; this one isn't: the file was copied.

copy-file is akin to a library function.  A library function should
never decide on its own when to call it quits, because a library
function never has enough context to do TRT.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 18:35           ` Eli Zaretskii
@ 2013-12-22 18:54             ` Paul Eggert
  2013-12-22 20:32               ` Artem Chuprina
  2013-12-22 21:00               ` Eli Zaretskii
  0 siblings, 2 replies; 33+ messages in thread
From: Paul Eggert @ 2013-12-22 18:54 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ran, 16133

Eli Zaretskii wrote:
> A library function should
> never decide on its own when to call it quits

Copy-file signals errors in many circumstances;
this is just one of them.  If the goal is to have
a library function that does not "decide on its
own when to call it quits", then the function should
return a value indicating the error, rather than
throwing an exception.  That might be too much of
a change to copy-file, but we could have a variant
of copy-file that does that, suitable for library
use.

> It is only justified to do that when the
> error is fatal; this one isn't: the file was copied.

The contents of the file were copied, but the permissions
were not.  The specification of copy-file says that it
copies both contents and permissions.

I would favor changing the specification of copy-file,
so that it doesn't copy the permissions
unless its 6th argument is non-nil.  We could change
the name of the 6th argument to PRESERVE-PERMISSIONS.
The 6th argument already governs how permissions are
preserved, so this would be a reasonable change.  I don't
think it'd hurt existing applications, since the default
permissions when creating a file would be the same as
it is now -- the only change would be that copy-file
wouldn't attempt to change the permissions of an already-existing
file unless PRESERVE-PERMISSIONS is non-nil.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 15:50       ` Artem Chuprina
@ 2013-12-22 19:03         ` Paul Eggert
  2013-12-22 20:13           ` Artem Chuprina
  0 siblings, 1 reply; 33+ messages in thread
From: Paul Eggert @ 2013-12-22 19:03 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

Artem Chuprina wrote:
> With copy-file in emacs24 I just cannot write such code

It's certainly *possible* to write such code: just catch
the error and continue.

> much code that used copy-file before, just stopped working.  (I've
> mentioned org-mobile-push, and emacs' own file backup code

This is a more serious matter, but it's a judgment call as to
whether it stopped working or started working.  Some users
want permissions to be saved, as well as contents.  Others
don't.  Emacs should support both usages.

I'm still puzzled as to why you're observing the problem and
I am not.  Can you do an 'strace' on an Emacs that's exhibiting
the problem?  It'd be helpful to see exactly which system calls
are being invoked, and the errno value for the failing one.
Possibly there is a small workaround here that wouldn't require
redesining copy-file.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 19:03         ` Paul Eggert
@ 2013-12-22 20:13           ` Artem Chuprina
  2013-12-23 23:58             ` Paul Eggert
  0 siblings, 1 reply; 33+ messages in thread
From: Artem Chuprina @ 2013-12-22 20:13 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 16133

Paul Eggert -> Artem Chuprina  @ Sun, 22 Dec 2013 11:03:24 -0800:

 >> With copy-file in emacs24 I just cannot write such code

 PE> It's certainly *possible* to write such code: just catch
 PE> the error and continue.

Not.  It's possible to write code that does the same, but it would be
MUCH more code.  Just to fight bad default behavior.  It's like instead of

#!/bin/sh -e
cp ....

you should EVER write

#!/bin/sh -e
cp ... || rc=$?
case $rc in
5,7,13,17) ;; # ignore chown, chmod, chattr and like problems
*) exit $rc
esac

Almost nobody almost never wants this.  

 >> much code that used copy-file before, just stopped working.  (I've
 >> mentioned org-mobile-push, and emacs' own file backup code

 PE> This is a more serious matter, but it's a judgment call as to
 PE> whether it stopped working or started working.  Some users
 PE> want permissions to be saved, as well as contents.  Others
 PE> don't.  Emacs should support both usages.

I've seen insisting on chown/chmod success when copying in ONE task
only: rsync backup of the WHOLE system.  I've never seen somebody who
will try to use emacs' copy-file for that task.

I think that if you want Emacs to support such a strange behavior, it
would be OK.  But NOT as default behavior.  As you appeal to GNU cp, see
its default behavior: BY DEFAULT it TRIES to save permissions and
owner/group, but DOES NOT fail if cannot.  If you personally insist that
this should be failure, cp allows this to you, but you must express this
EXPLICITLY.

I'm sure that copy-file should behave in this style.

 PE> I'm still puzzled as to why you're observing the problem and
 PE> I am not.  Can you do an 'strace' on an Emacs that's exhibiting
 PE> the problem?  It'd be helpful to see exactly which system calls
 PE> are being invoked, and the errno value for the failing one.
 PE> Possibly there is a small workaround here that wouldn't require
 PE> redesining copy-file.

$ touch testfile
$ strace -otrace emacs24 -Q
M-: (copy-file "testfile" "c1/testfile")
Debugger entered--Lisp error: (file-error "Doing chmod" "operation not permitted" $
  copy-file("testfile" "c1/testfile")
  eval((copy-file "testfile" "c1/testfile") nil)
  eval-expression((copy-file "testfile" "c1/testfile") nil)
  call-interactively(eval-expression nil nil)
C-x C-c

Relevant part of trace:
stat64("/home/ran/c1/testfile", 0xbeb0ff38) = -1 ENOENT (No such file or directory)
lstat64("/home/ran/c1/testfile", 0xbeb0ff20) = -1 ENOENT (No such file or directory)
open("/home/ran/testfile", O_RDONLY|O_LARGEFILE) = 4
fstat64(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
open("/home/ran/c1/testfile", O_WRONLY|O_CREAT|O_EXCL|O_TRUNC|O_LARGEFILE, 0644) = 5
read(4, "", 16384)                      = 0
fchmod(5, 0644)                         = -1 EPERM (Operation not permitted)
stat64("/usr/share/emacs/24.3/lisp/debug.elc", 0xbeb0fbe8) = -1 ENOENT (No such file or directory)
... (search for debugger)
stat64("/usr/share/emacs/24.3/lisp/emacs-lisp/debug.elc", {st_mode=S_IFREG|0644, st_size=27568, ...}) = 0
open("/usr/share/emacs/24.3/lisp/emacs-lisp/debug.elc", O_RDONLY|O_LARGEFILE) = 6

Indeed, after your request, I've tried to reproduce the bug on my
netbook, and for the first time could not.  Then I thought a little
more, and mounted FAT partition with options similar to those on Android
in question, that is,

mount -o fmask=0700,dmask=0700 /dev/sda1 /fat

(on Android they are exactly rw,dirsync,nosuid,nodev,noexec,relatime,uid=1000,gid=1015,fmask=0702,dmask=0702,allow_utime=0020,codepage=cp437,iocharset=iso8859-1,shortname=mixed,utf8,errors=remount-ro)

The problem reproduced.  

Further investigation shows that with (very reasonable on a multi-user
Linux system) mount options for VFAT

mount -o fmask=0111,dmask=0 /dev/sda1 /fat

(that is, all files belong to root:root, files have permissions 0666,
directories - 0777) we have the same problem.

That is, relevant part is that the resulting file does not belong to me
but I can write to it, I can create files there, but I cannot change
their metainfo, because they are not mine.  On Android I can write
through group permissions, in this case - through other permissions,
effect is the same.

If filesystem belongs to me, fchmod silently succeeds (permissions don't
change, of cause).  The problem also not reproduced if filesystem is
mounted with quiet option (it is designed just for this).  It is also
very reasonable on Linux systems when mounting FAT, but Android does not
use this option when mounting SD cards.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 18:54             ` Paul Eggert
@ 2013-12-22 20:32               ` Artem Chuprina
  2013-12-22 21:00               ` Eli Zaretskii
  1 sibling, 0 replies; 33+ messages in thread
From: Artem Chuprina @ 2013-12-22 20:32 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 16133

Paul Eggert -> Eli Zaretskii  @ Sun, 22 Dec 2013 10:54:48 -0800:

 PE> I would favor changing the specification of copy-file,
 PE> so that it doesn't copy the permissions
 PE> unless its 6th argument is non-nil.  We could change
 PE> the name of the 6th argument to PRESERVE-PERMISSIONS.
 PE> The 6th argument already governs how permissions are
 PE> preserved, so this would be a reasonable change.  I don't
 PE> think it'd hurt existing applications, since the default
 PE> permissions when creating a file would be the same as
 PE> it is now -- the only change would be that copy-file
 PE> wouldn't attempt to change the permissions of an already-existing
 PE> file unless PRESERVE-PERMISSIONS is non-nil.

I'd prefer copy-file behavior just like cp's.  That is, ever try to
preserve permissions, owner, ACLs etc, but by default throw an error
only if file CONTENT is not copied successfully.  Metainformation copy
failures should throw error on explicit request.  Probably, it would be
reasonable to have not several parameters, but one parameters
THROW-ERROR-ON(-META) with a list of flags, like '(acl owner times), t
for all, nil (default) for none.  Also like cp, it should NOT try to
copy times by default, only on explicit request.  But also, request to
copy times in NOT yet a reason to throw error.

For backward compatibility, it has sense to keep PRESERVE-UID-GID, but
also for backward compatibility, with BACKWARD-COMPATIBLE behavior (that
is, NOT throw an exception, AGGRHH!).  If we don't bother backward
compatibility, it would be better to remove PRESERVE-UID-GID entirely,
replacing it with THROW-ERROR-ON.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 18:54             ` Paul Eggert
  2013-12-22 20:32               ` Artem Chuprina
@ 2013-12-22 21:00               ` Eli Zaretskii
  1 sibling, 0 replies; 33+ messages in thread
From: Eli Zaretskii @ 2013-12-22 21:00 UTC (permalink / raw)
  To: Paul Eggert; +Cc: ran, 16133

> Date: Sun, 22 Dec 2013 10:54:48 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 16133@debbugs.gnu.org, ran@lasgalen.net
> 
> Eli Zaretskii wrote:
> > A library function should
> > never decide on its own when to call it quits
> 
> Copy-file signals errors in many circumstances;
> this is just one of them.  If the goal is to have
> a library function that does not "decide on its
> own when to call it quits", then the function should
> return a value indicating the error, rather than
> throwing an exception.  That might be too much of
> a change to copy-file, but we could have a variant
> of copy-file that does that, suitable for library
> use.

It could be a workable alternative indeed, but I don't see why
copy-file couldn't do it itself: it currently returns no useful value,
so no code relies on its return value.

> I would favor changing the specification of copy-file,
> so that it doesn't copy the permissions
> unless its 6th argument is non-nil.

The case in point is when you want to copy permission, if possible,
but will settle for a copy without permission if not.  IOW, no need to
give up without trying.

Perhaps we should have a special value of the 6th argument which means
treat a failure to copy permissions as fatal.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-22 20:13           ` Artem Chuprina
@ 2013-12-23 23:58             ` Paul Eggert
  2013-12-24  6:52               ` Artem Chuprina
  0 siblings, 1 reply; 33+ messages in thread
From: Paul Eggert @ 2013-12-23 23:58 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

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

Artem Chuprina wrote:
> As you appeal to GNU cp, see
> its default behavior: BY DEFAULT it TRIES to save permissions and
> owner/group

No, by default GNU cp does not try to copy either owner/group
or permissions to an existing destination.  It does not invoke
chmod or chown unless you use something like 'cp -p'.

> I can create files there, but I cannot change
> their metainfo, because they are not mine.

This is an unusual setup, at least for me.  If I create a file,
I should be able to change its metainformation.  I expect this
setup will cause problems with other applications, not just Emacs.
GNU tar would be one example.

That being said, we should be able to work around the problem
by having copy-file behave more like 'cp'.  That is, copy-file
should not invoke chmod by default; it should invoke chmod only
if it's told to preserve permissions (or preserve ownership,
since that often involves temporarily revoking permissions for
security reasons).  That way, plain copy-file should work with
your setup, although you'll still have trouble with
copy-file with the last arg t (which asks to copy permissions).

Attached is a proposed patch to do that, against trunk bzr 115721.
Does it solve your problem?



[-- Attachment #2: copy-file.diff --]
[-- Type: text/x-patch, Size: 15512 bytes --]

=== modified file 'doc/lispref/ChangeLog'
--- doc/lispref/ChangeLog	2013-12-23 11:27:29 +0000
+++ doc/lispref/ChangeLog	2013-12-23 23:53:26 +0000
@@ -1,3 +1,8 @@
+2013-12-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Plain copy-file no longer chmods an existing destination (Bug#16133).
+	* files.texi (Changing Files): Document this.
+
 2013-12-23  Xue Fuqiao  <xfq.free@gmail.com>
 
 	* eval.texi (Special Forms): Document `special-form-p'.

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2013-12-23 08:50:31 +0000
+++ doc/lispref/files.texi	2013-12-23 23:53:26 +0000
@@ -1563,8 +1563,6 @@
 interactive call, a prefix argument specifies a non-@code{nil} value
 for @var{time}.
 
-This function copies the file modes, too.
-
 If argument @var{preserve-uid-gid} is @code{nil}, we let the operating
 system decide the user and group ownership of the new file (this is
 usually set to the user running Emacs).  If @var{preserve-uid-gid} is
@@ -1572,10 +1570,11 @@
 file.  This works only on some operating systems, and only if you have
 the correct permissions to do so.
 
-If the optional argument @var{preserve-extended-attributes} is
-non-@code{nil}, and Emacs has been built with the appropriate support,
-this function attempts to copy the file's extended attributes, such as
-its SELinux context and ACL entries (@pxref{File Attributes}).
+If the optional argument @var{preserve-permissions} is non-@code{nil},
+this function copies the file's permissions, such as its file modes,
+its SELinux context, and ACL entries (@pxref{File Attributes}).
+Otherwise, if the destination is created its file permission bits are
+those of the source, masked by the default file permissions.
 @end deffn
 
 @deffn Command make-symbolic-link filename newname  &optional ok-if-exists

=== modified file 'etc/ChangeLog'
--- etc/ChangeLog	2013-12-23 22:27:49 +0000
+++ etc/ChangeLog	2013-12-23 23:53:26 +0000
@@ -1,3 +1,8 @@
+2013-12-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Plain copy-file no longer chmods an existing destination (Bug#16133).
+	* NEWS: Document this.
+
 2013-12-23  Teodor Zlatanov  <tzz@lifelogs.com>
 
 	* NEWS: Updated for `gnutls-verify-error', cfengine-mode, and

=== modified file 'etc/NEWS'
--- etc/NEWS	2013-12-23 13:05:27 +0000
+++ etc/NEWS	2013-12-23 23:53:26 +0000
@@ -889,6 +889,12 @@
 `file-extended-attributes'.  The attributes can be applied to another
 file using `set-file-extended-attributes'.
 
+** By default `copy-file' no longer copies file permission bits to an
+existing destination; and it sets the file permission bits of a newly
+created destination to those of the source, masked by the default file
+permissions.  To copy the file permission bits, pass t as the
+PRESERVE-PERMISSIONS argument of `copy-file'.
+
 ** `visited-file-modtime' now returns -1 for nonexistent files.
 Formerly it returned a list (-1 LOW USEC PSEC), but this was ambiguous
 in the presence of files with negative time stamps.
@@ -1030,8 +1036,8 @@
 
 +++
 *** The 6th argument to `copy-file' has been renamed to
-PRESERVE-EXTENDED-ATTRIBUTES as it now handles both SELinux context
-and ACL entries.
+PRESERVE-PERMISSIONS as it now handles ACL entries and the traditional
+Unix file permission bits as well as SELinux context.
 
 +++
 *** The function `file-ownership-preserved-p' now has an optional

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2013-12-23 19:24:25 +0000
+++ src/ChangeLog	2013-12-23 23:53:26 +0000
@@ -1,3 +1,20 @@
+2013-12-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Plain copy-file no longer chmods an existing destination (Bug#16133).
+	* fileio.c (realmask): Now a static var, not a local.
+	(barf_or_query_if_file_exists): New arg KNOWN_TO_EXIST.
+	Remove arg STATPTR.  All uses changed.
+	(Fcopy_file): Do not alter permissions of existing destinations,
+	unless PRESERVE-PERMISSIONS (renamed from
+	PRESERVE-EXTENDED-ATTRIBUTES) is non-nil.
+	Avoid race when testing for existing destinations and for
+	when input and output files are the same.
+	If changing the group fails, adjust both default and
+	preserved permissions so that access is not granted to the
+	wrong group.
+	(Fset_default_file_modes, init_fileio): Update realmask.
+	(Fdefault_file_modes): Use realmask instead of calling umask.
+
 2013-12-23  Eli Zaretskii  <eliz@gnu.org>
 
 	* xdisp.c (tool_bar_height): Use WINDOW_PIXEL_WIDTH to set up the

=== modified file 'src/fileio.c'
--- src/fileio.c	2013-12-17 17:46:31 +0000
+++ src/fileio.c	2013-12-23 23:53:26 +0000
@@ -95,6 +95,9 @@
 /* True during writing of auto-save files.  */
 static bool auto_saving;
 
+/* Emacs's real umask.  */
+static mode_t realmask;
+
 /* Nonzero umask during creation of auto-save directories.  */
 static mode_t auto_saving_dir_umask;
 
@@ -1858,20 +1861,16 @@
 }
 \f
 /* Signal an error if the file ABSNAME already exists.
+   If KNOWN_TO_EXIST, the file is known to exist.
+   QUERYSTRING is a name for the action that is being considered
+   to alter the file.
    If INTERACTIVE, ask the user whether to proceed,
    and bypass the error if the user says to go ahead.
-   QUERYSTRING is a name for the action that is being considered
-   to alter the file.
-
-   *STATPTR is used to store the stat information if the file exists.
-   If the file does not exist, STATPTR->st_mode is set to 0.
-   If STATPTR is null, we don't store into it.
-
    If QUICK, ask for y or n, not yes or no.  */
 
 static void
-barf_or_query_if_file_exists (Lisp_Object absname, const char *querystring,
-			      bool interactive, struct stat *statptr,
+barf_or_query_if_file_exists (Lisp_Object absname, bool known_to_exist,
+			      const char *querystring, bool interactive,
 			      bool quick)
 {
   Lisp_Object tem, encoded_filename;
@@ -1880,14 +1879,16 @@
 
   encoded_filename = ENCODE_FILE (absname);
 
-  /* `stat' is a good way to tell whether the file exists,
-     regardless of what access permissions it has.  */
-  if (lstat (SSDATA (encoded_filename), &statbuf) >= 0)
+  if (! known_to_exist && lstat (SSDATA (encoded_filename), &statbuf) == 0)
     {
       if (S_ISDIR (statbuf.st_mode))
 	xsignal2 (Qfile_error,
 		  build_string ("File is a directory"), absname);
+      known_to_exist = true;
+    }
 
+  if (known_to_exist)
+    {
       if (! interactive)
 	xsignal2 (Qfile_already_exists,
 		  build_string ("File already exists"), absname);
@@ -1902,15 +1903,7 @@
       if (NILP (tem))
 	xsignal2 (Qfile_already_exists,
 		  build_string ("File already exists"), absname);
-      if (statptr)
-	*statptr = statbuf;
-    }
-  else
-    {
-      if (statptr)
-	statptr->st_mode = 0;
-    }
-  return;
+    }
 }
 
 DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
@@ -1937,16 +1930,21 @@
 If PRESERVE-UID-GID is non-nil, we try to transfer the
 uid and gid of FILE to NEWNAME.
 
-If PRESERVE-EXTENDED-ATTRIBUTES is non-nil, we try to copy additional
-attributes of FILE to NEWNAME, such as its SELinux context and ACL
-entries (depending on how Emacs was built).  */)
-  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists, Lisp_Object keep_time, Lisp_Object preserve_uid_gid, Lisp_Object preserve_extended_attributes)
+If PRESERVE-PERMISSIONS is non-nil, copy permissions of FILE to NEWNAME;
+this includes the file modes, along with ACL entries and SELinux
+context if present.  Otherwise, if NEWNAME is created its file
+permission bits are those of FILE, masked by the default file
+permissions.  */)
+  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists,
+   Lisp_Object keep_time, Lisp_Object preserve_uid_gid,
+   Lisp_Object preserve_permissions)
 {
-  struct stat out_st;
   Lisp_Object handler;
   struct gcpro gcpro1, gcpro2, gcpro3, gcpro4;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object encoded_file, encoded_newname;
+  bool already_exists = false;
+  mode_t new_mask;
 #if HAVE_LIBSELINUX
   security_context_t con;
   int conlength = 0;
@@ -1981,22 +1979,20 @@
   if (!NILP (handler))
     RETURN_UNGCPRO (call7 (handler, Qcopy_file, file, newname,
 			   ok_if_already_exists, keep_time, preserve_uid_gid,
-			   preserve_extended_attributes));
+			   preserve_permissions));
 
   encoded_file = ENCODE_FILE (file);
   encoded_newname = ENCODE_FILE (newname);
 
+#ifdef WINDOWSNT
   if (NILP (ok_if_already_exists)
       || INTEGERP (ok_if_already_exists))
-    barf_or_query_if_file_exists (newname, "copy to it",
-				  INTEGERP (ok_if_already_exists), &out_st, 0);
-  else if (stat (SSDATA (encoded_newname), &out_st) < 0)
-    out_st.st_mode = 0;
+    barf_or_query_if_file_exists (newname, false, "copy to it",
+				  INTEGERP (ok_if_already_exists), false);
 
-#ifdef WINDOWSNT
   result = w32_copy_file (SSDATA (encoded_file), SSDATA (encoded_newname),
 			  !NILP (keep_time), !NILP (preserve_uid_gid),
-			  !NILP (preserve_extended_attributes));
+			  !NILP (preserve_permissions));
   switch (result)
     {
     case -1:
@@ -2022,7 +2018,7 @@
   if (fstat (ifd, &st) != 0)
     report_file_error ("Input file status", file);
 
-  if (!NILP (preserve_extended_attributes))
+  if (!NILP (preserve_permissions))
     {
 #if HAVE_LIBSELINUX
       if (is_selinux_enabled ())
@@ -2034,32 +2030,46 @@
 #endif
     }
 
-  if (out_st.st_mode != 0
-      && st.st_dev == out_st.st_dev && st.st_ino == out_st.st_ino)
-    report_file_errno ("Input and output files are the same",
-		       list2 (file, newname), 0);
-
   /* We can copy only regular files.  */
   if (!S_ISREG (st.st_mode))
     report_file_errno ("Non-regular file", file,
 		       S_ISDIR (st.st_mode) ? EISDIR : EINVAL);
 
-  {
 #ifndef MSDOS
-    int new_mask = st.st_mode & (!NILP (preserve_uid_gid) ? 0600 : 0666);
+  new_mask = st.st_mode & 0777;
+  if (!NILP (preserve_uid_gid) || !NILP (preserve_permissions))
+    new_mask &= 0700;
 #else
-    int new_mask = S_IREAD | S_IWRITE;
+  new_mask = S_IREAD | S_IWRITE;
 #endif
-    ofd = emacs_open (SSDATA (encoded_newname),
-		      (O_WRONLY | O_TRUNC | O_CREAT
-		       | (NILP (ok_if_already_exists) ? O_EXCL : 0)),
-		      new_mask);
-  }
+
+  ofd = emacs_open (SSDATA (encoded_newname), O_WRONLY | O_CREAT | O_EXCL,
+		    new_mask);
+  if (ofd < 0 && errno == EEXIST)
+    {
+      if (NILP (ok_if_already_exists) || INTEGERP (ok_if_already_exists))
+	barf_or_query_if_file_exists (newname, true, "copy to it",
+				      INTEGERP (ok_if_already_exists), false);
+      already_exists = true;
+      ofd = emacs_open (SSDATA (encoded_newname), O_WRONLY, 0);
+    }
   if (ofd < 0)
     report_file_error ("Opening output file", newname);
 
   record_unwind_protect_int (close_file_unwind, ofd);
 
+  if (already_exists)
+    {
+      struct stat out_st;
+      if (fstat (ofd, &out_st) != 0)
+	report_file_error ("Output file status", newname);
+      if (st.st_dev == out_st.st_dev && st.st_ino == out_st.st_ino)
+	report_file_errno ("Input and output files are the same",
+			   list2 (file, newname), 0);
+      if (ftruncate (ofd, 0) != 0)
+	report_file_error ("Truncating output file", newname);
+    }
+
   immediate_quit = 1;
   QUIT;
   while ((n = emacs_read (ifd, buf, sizeof buf)) > 0)
@@ -2071,26 +2081,41 @@
   /* Preserve the original file permissions, and if requested, also its
      owner and group.  */
   {
-    mode_t mode_mask = 07777;
+    mode_t preserved_permissions = st.st_mode & 07777;
+    mode_t default_permissions = st.st_mode & 0777 & ~realmask;
     if (!NILP (preserve_uid_gid))
       {
 	/* Attempt to change owner and group.  If that doesn't work
 	   attempt to change just the group, as that is sometimes allowed.
 	   Adjust the mode mask to eliminate setuid or setgid bits
-	   that are inappropriate if the owner and group are wrong.  */
+	   or group permissions bits that are inappropriate if the
+	   owner or group are wrong.  */
 	if (fchown (ofd, st.st_uid, st.st_gid) != 0)
 	  {
-	    mode_mask &= ~06000;
 	    if (fchown (ofd, -1, st.st_gid) == 0)
-	      mode_mask |= 02000;
+	      preserved_permissions &= ~04000;
+	    else
+	      {
+		preserved_permissions &= ~06000;
+
+		/* Copy the other bits to the group bits, since the
+		   group is wrong.  */
+		preserved_permissions &= ~070;
+		preserved_permissions |= (preserved_permissions & 7) << 3;
+		default_permissions &= ~070;
+		default_permissions |= (default_permissions & 7) << 3;
+	      }
 	  }
       }
 
-    switch (!NILP (preserve_extended_attributes)
+    switch (!NILP (preserve_permissions)
 	    ? qcopy_acl (SSDATA (encoded_file), ifd,
 			 SSDATA (encoded_newname), ofd,
-			 st.st_mode & mode_mask)
-	    : fchmod (ofd, st.st_mode & mode_mask))
+			 preserved_permissions)
+	    : (already_exists
+	       || (new_mask & ~realmask) == default_permissions)
+	    ? 0
+	    : fchmod (ofd, default_permissions))
       {
       case -2: report_file_error ("Copying permissions from", file);
       case -1: report_file_error ("Copying permissions to", newname);
@@ -2307,8 +2332,8 @@
 #endif
   if (NILP (ok_if_already_exists)
       || INTEGERP (ok_if_already_exists))
-    barf_or_query_if_file_exists (newname, "rename to it",
-				  INTEGERP (ok_if_already_exists), 0, 0);
+    barf_or_query_if_file_exists (newname, false, "rename to it",
+				  INTEGERP (ok_if_already_exists), false);
   if (rename (SSDATA (encoded_file), SSDATA (encoded_newname)) < 0)
     {
       int rename_errno = errno;
@@ -2387,8 +2412,8 @@
 
   if (NILP (ok_if_already_exists)
       || INTEGERP (ok_if_already_exists))
-    barf_or_query_if_file_exists (newname, "make it a new name",
-				  INTEGERP (ok_if_already_exists), 0, 0);
+    barf_or_query_if_file_exists (newname, false, "make it a new name",
+				  INTEGERP (ok_if_already_exists), false);
 
   unlink (SSDATA (newname));
   if (link (SSDATA (encoded_file), SSDATA (encoded_newname)) < 0)
@@ -2449,8 +2474,8 @@
 
   if (NILP (ok_if_already_exists)
       || INTEGERP (ok_if_already_exists))
-    barf_or_query_if_file_exists (linkname, "make it a link",
-				  INTEGERP (ok_if_already_exists), 0, 0);
+    barf_or_query_if_file_exists (linkname, false, "make it a link",
+				  INTEGERP (ok_if_already_exists), false);
   if (symlink (SSDATA (encoded_filename), SSDATA (encoded_linkname)) < 0)
     {
       /* If we didn't complain already, silently delete existing file.  */
@@ -3137,10 +3162,17 @@
 This setting is inherited by subprocesses.  */)
   (Lisp_Object mode)
 {
+  mode_t oldrealmask, oldumask, newumask;
   CHECK_NUMBER (mode);
-
-  umask ((~ XINT (mode)) & 0777);
-
+  oldrealmask = realmask;
+  newumask = ~ XINT (mode) & 0777;
+
+  block_input ();
+  realmask = newumask;
+  oldumask = umask (newumask);
+  unblock_input ();
+
+  eassert (oldumask == oldrealmask);
   return Qnil;
 }
 
@@ -3149,14 +3181,7 @@
 The value is an integer.  */)
   (void)
 {
-  mode_t realmask;
   Lisp_Object value;
-
-  block_input ();
-  realmask = umask (0);
-  umask (realmask);
-  unblock_input ();
-
   XSETINT (value, (~ realmask) & 0777);
   return value;
 }
@@ -4697,7 +4722,7 @@
   filename = Fexpand_file_name (filename, Qnil);
 
   if (!NILP (mustbenew) && !EQ (mustbenew, Qexcl))
-    barf_or_query_if_file_exists (filename, "overwrite", 1, 0, 1);
+    barf_or_query_if_file_exists (filename, false, "overwrite", true, true);
 
   if (STRINGP (visit))
     visit_file = Fexpand_file_name (visit, Qnil);
@@ -5765,6 +5790,9 @@
 void
 init_fileio (void)
 {
+  realmask = umask (0);
+  umask (realmask);
+
   valid_timestamp_file_system = 0;
 
   /* fsync can be a significant performance hit.  Often it doesn't


^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-23 23:58             ` Paul Eggert
@ 2013-12-24  6:52               ` Artem Chuprina
  2013-12-24  9:58                 ` Andreas Schwab
                                   ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Artem Chuprina @ 2013-12-24  6:52 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 16133

Paul Eggert -> Artem Chuprina  @ Mon, 23 Dec 2013 15:58:29 -0800:

 >> As you appeal to GNU cp, see
 >> its default behavior: BY DEFAULT it TRIES to save permissions and
 >> owner/group

 PE> No, by default GNU cp does not try to copy either owner/group
 PE> or permissions to an existing destination.  It does not invoke
 PE> chmod or chown unless you use something like 'cp -p'.

You are wrong.

zsh% umask 
002
zsh% touch testfile
zsh% ls -l testfile
-rw-rw-r-- 1 ran ran 0 Дек 24 10:37 testfile
zsh% chmod 600 testfile
zsh% cp testfile testfile.copy
zsh% ls -l testfile.copy 
-rw------- 1 ran ran 0 Дек 24 10:38 testfile.copy

Debian GNU/Linux 7.3 (wheezy)

 >> I can create files there, but I cannot change
 >> their metainfo, because they are not mine.

 PE> This is an unusual setup, at least for me.
 PE> If I create a file, I should be able to change its metainformation.

Key phrase: "at least for me".  This is usual setup with FAT filesystems
on every multiuser system.  Because FAT cannot keep owner and there are
multiple users, it is mounted so that all the files belong to root or
another system user, not to some real user.

 PE> I expect this setup will cause problems with other applications,
 PE> not just Emacs.  GNU tar would be one example.

You are wrong again.

zsh% tar tvf ~/testfiles.tar
-rw------- ran/ran           0 2013-12-24 10:37 testfile
-rw------- ran/ran           0 2013-12-24 10:38 testfile.copy
zsh% cd /fat 
zsh% tar xvf ~/testfiles.tar 
testfile
testfile.copy
zsh% echo $?
0
zsh% ls -l testfile*
-rw-rw-rw- 1 root root 0 Дек 24 10:37 testfile
-rw-rw-rw- 1 root root 0 Дек 24 10:38 testfile.copy

 PE> That being said, we should be able to work around the problem
 PE> by having copy-file behave more like 'cp'.  That is, copy-file
 PE> should not invoke chmod by default; it should invoke chmod only
 PE> if it's told to preserve permissions (or preserve ownership,
 PE> since that often involves temporarily revoking permissions for
 PE> security reasons).  That way, plain copy-file should work with
 PE> your setup, although you'll still have trouble with
 PE> copy-file with the last arg t (which asks to copy permissions).

This is also reasonable behavior, but not the best one, and it is
inconsistent with such of GNU cp and GNU tar, as proved above.

 PE> Attached is a proposed patch to do that, against trunk bzr 115721.
 PE> Does it solve your problem?

I'll try to check it today.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-24  6:52               ` Artem Chuprina
@ 2013-12-24  9:58                 ` Andreas Schwab
  2013-12-24 10:22                   ` Artem Chuprina
  2013-12-24 16:51                 ` Artem Chuprina
  2013-12-29 18:31                 ` Paul Eggert
  2 siblings, 1 reply; 33+ messages in thread
From: Andreas Schwab @ 2013-12-24  9:58 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: Paul Eggert, 16133

Artem Chuprina <ran@lasgalen.net> writes:

> zsh% cp testfile testfile.copy

That's not an existing destination.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-24  9:58                 ` Andreas Schwab
@ 2013-12-24 10:22                   ` Artem Chuprina
  2013-12-24 17:39                     ` Paul Eggert
  0 siblings, 1 reply; 33+ messages in thread
From: Artem Chuprina @ 2013-12-24 10:22 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Paul Eggert, 16133

Andreas Schwab -> Artem Chuprina  @ Tue, 24 Dec 2013 10:58:46 +0100:

 >> zsh% cp testfile testfile.copy

 AS> That's not an existing destination.

Well, your correction is right.  cp's behavior is more complex than
"just try to save permissions".  When the target file exists, cp does
not try unless explicitly asked to.  But when creating file, it does
(that is, file's permissions are copied from source, not just left as
set by umask).





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-24  6:52               ` Artem Chuprina
  2013-12-24  9:58                 ` Andreas Schwab
@ 2013-12-24 16:51                 ` Artem Chuprina
  2013-12-29 18:31                 ` Paul Eggert
  2 siblings, 0 replies; 33+ messages in thread
From: Artem Chuprina @ 2013-12-24 16:51 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 16133

Artem Chuprina <ran@lasgalen.net> writes:

>  PE> Attached is a proposed patch to do that, against trunk bzr 115721.
>  PE> Does it solve your problem?
>
> I'll try to check it today.

Sorry, I could not.  Now I'm on my vacation, far from home, and I cannot
check it for 3 weeks.





^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-24 10:22                   ` Artem Chuprina
@ 2013-12-24 17:39                     ` Paul Eggert
  0 siblings, 0 replies; 33+ messages in thread
From: Paul Eggert @ 2013-12-24 17:39 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133

Artem Chuprina wrote:
> When the target file exists, cp does
> not try unless explicitly asked to.  But when creating file, it does
> (that is, file's permissions are copied from source, not just left as
> set by umask).

Even when creating a file, GNU cp by default does not use 'chmod'
to copy the file's permissions (12 bits).  Instead, it merely
creates the file with the permissions of the source
as modified by the umask (9 bits).  That is why it doesn't
run into a problem in your setup.

The proposed patch would cause GNU Emacs to act similarly,
which is why I expect it to work for you.






^ permalink raw reply	[flat|nested] 33+ messages in thread

* bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem
  2013-12-24  6:52               ` Artem Chuprina
  2013-12-24  9:58                 ` Andreas Schwab
  2013-12-24 16:51                 ` Artem Chuprina
@ 2013-12-29 18:31                 ` Paul Eggert
  2 siblings, 0 replies; 33+ messages in thread
From: Paul Eggert @ 2013-12-29 18:31 UTC (permalink / raw)
  To: Artem Chuprina; +Cc: 16133-done

Artem Chuprina wrote:
>   PE> I expect this setup will cause problems with other applications,
>   PE> not just Emacs.  GNU tar would be one example.
>
> You are wrong again.

Well, as Andreas mentioned, I was right about cp, so the
"again" in your remark is incorrect; but you are correct about
'tar'; it uses a trick in which it sets the umask to 0 to
avoid the need for fchmod.

Still, many programs don't use that trick, and you'll run
into problems with these programs.  GNU cpio would be one
example; if you do something like "find . -print | cpio -pdmuv .../dest",
where "dest" is in that file system, cpio will report an error
because it can't change the permission of the file that it creates.

In reviewing the email in this bug report it appears that we do
have a real problem here, and that a "real" solution will probably
require redesigning the API significantly so that there's a
copy-file variant that doesn't throw errors, or something like that.
Right now we're just trying to fix bugs, though, so I installed
the more-conservative change, where copy-file doesn't try to fchmod
an existing file -- this causes copy-file to act more like plain 'cp'
and I expect it's what more copy-file users would expect anyway.





^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2013-12-29 18:31 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-13 19:51 bug#16133: 24.3; copy-file fails on chmod when copying to FAT filesystem Artem Chuprina
2013-12-13 22:51 ` Glenn Morris
2013-12-13 22:55   ` Glenn Morris
2013-12-14 10:10   ` Artem Chuprina
2013-12-14 20:19     ` Glenn Morris
2013-12-14 20:46       ` Josh
2013-12-14 20:57         ` Eli Zaretskii
2013-12-14 21:21           ` Josh
2013-12-15  3:44             ` Eli Zaretskii
2013-12-14 20:55       ` Eli Zaretskii
2013-12-14 21:07       ` Achim Gratz
2013-12-15 14:38       ` Artem Chuprina
2013-12-16 14:15         ` Stefan Monnier
2013-12-20 23:27 ` Paul Eggert
2013-12-22  0:01 ` Paul Eggert
2013-12-22  3:47   ` Eli Zaretskii
2013-12-22  4:01     ` Paul Eggert
2013-12-22 15:50       ` Artem Chuprina
2013-12-22 19:03         ` Paul Eggert
2013-12-22 20:13           ` Artem Chuprina
2013-12-23 23:58             ` Paul Eggert
2013-12-24  6:52               ` Artem Chuprina
2013-12-24  9:58                 ` Andreas Schwab
2013-12-24 10:22                   ` Artem Chuprina
2013-12-24 17:39                     ` Paul Eggert
2013-12-24 16:51                 ` Artem Chuprina
2013-12-29 18:31                 ` Paul Eggert
2013-12-22 16:24       ` Eli Zaretskii
2013-12-22 17:37         ` Paul Eggert
2013-12-22 18:35           ` Eli Zaretskii
2013-12-22 18:54             ` Paul Eggert
2013-12-22 20:32               ` Artem Chuprina
2013-12-22 21:00               ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.