bug#30534: cp - Possible bugs when not preserving mode (explicit) and when copying special files

[Pádraig Brady]

On 19/02/18 02:22, Declercq Laurent wrote:
> I think that I did found at least two bugs in cp(1) command when the 
> --no-preserve=mode option is involved and when copying special file. I 
> describe each of them below.
> 
> 1. Mode set on special files seem to be wrong:
> 
> Original file to copy: prw-rw-rw- 1 root staff 0 févr. 18 18:59 spfile
> cp(1) command (run as root user): cp -r --no-preserve=mode spfile 
> spfile_copy
> 
> Current result:
> 
> prwxr-xr-x 1 root staff 0 févr. 18 22:01 spfile_copy
> 
> Expected result (considering UMASK 0022):
> 
> prw-r--r-- 1 root staff 0 févr. 18 22:01 spfile_copy
> 
> The current behavior is due to the fact that mode used is 0777 while 
> 0666 should be used for files.
> 
> Possible fix: Differentiate directories from files in the copy_internal 
> function.

Thanks for the clear details.
The attached should fix this up.

> 2. Non-permission bits are preserved, even when the --no-preserve=mode 
> option is involved.
> 
> Original file to copy: prwSrw-rw- 1 root staff 0 févr. 18 18:59 spfile
> cp(1) command (run as root user): cp -r --no-preserve=mode spfile 
> spfile_copy
> 
> Current result:
> 
> prwsr-xr-x 1 root staff 0 févr. 18 22:05 spfile_copy

I'm not seeing this. I get 'x' rather than 's' here (and '-' with the fix)

> Expected result (considering UMASK 0022 and without the first bug above):
> 
> prw-r--r-- 1 root staff 0 févr. 18 22:05 spfile_copy

thanks!
Pádraig

From 71e05111fa9dd6a4ad29630752d95289cb6d0274 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Mon, 19 Feb 2018 19:10:14 -0800
Subject: [PATCH] cp: set appropriate default permissions for special files

This issue was introduced in commit v8.19-145-g24ebca6

* src/copy.c (copy_internal): When setting default permissions
to use with --no-preserve=mode, only set executable bits for
directories or sockets.
* NEWS: Mention the fix.
* tests/cp/preserve-mode.sh: Add a test case.
Fixes https://bugs.gnu.org/30534
---
 NEWS  |  5 +
 src/copy.c|  6 --
 tests/cp/preserve-mode.sh | 10 +-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 8a9e09e..5fa6928 100644
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,11 @@ GNU coreutils NEWS-*- outline -*-
   that caused -u to sometimes override -n.
   [bug introduced with coreutils-7.1]
 
+  'cp -a --no-preserve=mode' now sets appropriate default permissions
+  for non regular files like fifos and character device nodes etc.
+  Previously it would have set executable bits on created special files.
+  [bug introduced with coreutils-8.20]
+
 
 * Noteworthy changes in release 8.29 (2017-12-27) [stable]
 
diff --git a/src/copy.c b/src/copy.c
index e050d41..233b498 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1378,7 +1378,7 @@ preserve_metadata:
 }
   else if (x->explicit_no_preserve_mode)
 {
-  if (set_acl (dst_name, dest_desc, 0666 & ~cached_umask ()) != 0)
+  if (set_acl (dst_name, dest_desc, MODE_RW_UGO & ~cached_umask ()) != 0)
 return_val = false;
 }
   else if (omitted_permissions)
@@ -2860,7 +2860,9 @@ copy_internal (char const *src_name, char const *dst_name,
 }
   else if (x->explicit_no_preserve_mode)
 {
-  if (set_acl (dst_name, -1, 0777 & ~cached_umask ()) != 0)
+  int default_permissions = S_ISDIR (src_mode) || S_ISSOCK (src_mode)
+? S_IRWXUGO : MODE_RW_UGO;
+  if (set_acl (dst_name, -1, default_permissions & ~cached_umask ()) != 0)
 return false;
 }
   else
diff --git a/tests/cp/preserve-mode.sh b/tests/cp/preserve-mode.sh
index 1cd173a..3b0aca8 100755
--- a/tests/cp/preserve-mode.sh
+++ b/tests/cp/preserve-mode.sh
@@ -19,7 +19,7 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ cp
 
-get_mode() { ls -ld "$1" | cut -b-10; }
+get_mode() { stat -c%f "$1"; }
 
 rm -f a b c
 umask 0022
@@ -47,4 +47,12 @@ chmod 600 a
 cp --no-preserve=mode --preserve=all a b || fail=1
 test "$(get_mode a)" = "$(get_mode b)" || fail=1
 
+#fifo test
+if mkfifo fifo; then
+  cp -a --no-preserve=mode fifo fifo_copy || fail=1
+  #ensure default perms set appropriate for non regular files
+  #which wasn't done between v8.20 and 8.29 inclusive
+  test "$(get_mode fifo)" = "$(get_mode fifo_copy)" || fail=1
+fi
+
 Exit $fail
-- 
2.9.3

bug#29617: `seq 1 --help' doesn't give help

[chadweisman]

On 2/19/2018 at 8:35 PM, "Pádraig Brady"  wrote:
>
>On 18/02/18 10:41, chadweis...@nym.hush.com wrote:
>> 
>>> On 12/08/2017 11:38 AM, Eric B wrote:
 Hello,

 I am using coreutils version 8.27 on Fedora, and I don't see 
>this fixed in 
 8.28's NEWS.

 $ seq 1 --help
 seq: invalid floating point argument: ‘--help’
 Try 'seq --help' for more information.
>> 
>>> Interesting bug!
>> 

 We should be able to put the options anywhere and not 
>necessarily before any 
 arguments.
>> 
>>> Yes, when possible.
>> 
 And even if not (e.g. POSIX conformance overrides,)
>> 
>>> POSIX does say you have to write 'foo -- --help' if you want to
>>> guarantee that --help is treated as a literal argument rather 
>than
>>> option, but it also says that the moment you specify '--help' 
>(or any
>>> other parameter starting with two dashes without the -- end-of-
>options
>>> parameter), you are already in undefined territory.  So we can 
>do
>>> whatever we want when encountering '--help' - which is part of 
>the
>>> reason WHY the GNU project prefers making 'foo args --help' 
>print help
>>> output where possible.
>> 
 --help should be handled specially to conform to the GNU 
>coding standards. [1]
>> 
>>> Yes.
>> 
>>> But the reason that it fails is because we use getopt_long(...,
>>> "+f:s:w") - where the leading '+' specifically requests that we 
>NOT
>>> allow option reordering.  Why? Because 'seq' is MOST useful if 
>it can
>>> parse negative numbers easily.  We deemed it more important to 
>support
>>> 'seq 2 -1 1' without requiring the user to write 'seq -- 2 -1 
>1' - but
>>> in doing so, it also means that we can't reorder options, so 
>any obvious
>>> non-option (like '1' in your example) makes all other parameters
>>> non-options (including '--help' in your example).
>> 
>>> It might be possible to do a two-pass parse over argv: one that 
>looks
>>> just for --help (and where treating -1 as an option is a no-
>op), and the
>>> second that actually parses things in order now that it knows --
>help is
>>> not present.  But that's a lot of code to add for a corner 
>case, so I
>>> won't be writing the patch; but I also won't turn it down if 
>someone
>>> else wants to write the patch.
>> 
>> Hello,
>> 
>> I've taken a stab at fixing this problem because it affects me 
>fairly often.
>> 
>> Instead of using a two-pass system, I check if any of the args 
>we scan are --help or --version and bail if we see either.  See 
>attached patch.
>> 
>> The bad side of this approach is that the -f, -s, and -w options 
>and their associated long options aren't handled so `seq 1 -w 2 
>10` still shows an error.  Also, it's a kludgy sort of fix, so I 
>completely understand why you wouldn't want to include it.  But at 
>least it's a step in the right direction to give help when we can 
>instead of an error.
>> 
>> Chad
>> 
>
>Thanks for looking at this again.
>You attached the wrong patch.
>

Wow, did I ever!  Sorry for that.

>A helper function to prescan argv to look for --help and call 
>usage()
>or --version and call version_etc(), while bailing itself
>if it encounters '--', does seem useful to have in seq.
>Though note it probably shouldn't support abbreviations like --h 
>etc.
>
>BTW other commands that use '+' with getopt() to exist option
>processing early are tr, basename, pathchk, printf.
>Though all those could interpret --option as a valid argument
>and so wouldn't be appropriate to treat like this.
>Likewise for all the commands the exec, like chroot, env, ...
>as --options are very often passed to the delegate commands.
>

I also tried a two-pass approach but it ended up being pretty messy.  I can't 
help but think using argp would make this particular problem easier to handle.

Chad

coreutils-seq.patch
Description: Binary data

bug#29617: `seq 1 --help' doesn't give help

[Pádraig Brady]

On 18/02/18 10:41, chadweis...@nym.hush.com wrote:
> 
>> On 12/08/2017 11:38 AM, Eric B wrote:
>>> Hello,
>>>
>>> I am using coreutils version 8.27 on Fedora, and I don't see this fixed in 
>>> 8.28's NEWS.
>>>
>>> $ seq 1 --help
>>> seq: invalid floating point argument: ‘--help’
>>> Try 'seq --help' for more information.
> 
>> Interesting bug!
> 
>>>
>>> We should be able to put the options anywhere and not necessarily before 
>>> any 
>>> arguments.
> 
>> Yes, when possible.
> 
>>> And even if not (e.g. POSIX conformance overrides,)
> 
>> POSIX does say you have to write 'foo -- --help' if you want to
>> guarantee that --help is treated as a literal argument rather than
>> option, but it also says that the moment you specify '--help' (or any
>> other parameter starting with two dashes without the -- end-of-options
>> parameter), you are already in undefined territory.  So we can do
>> whatever we want when encountering '--help' - which is part of the
>> reason WHY the GNU project prefers making 'foo args --help' print help
>> output where possible.
> 
>>> --help should be handled specially to conform to the GNU coding standards. 
>>> [1]
> 
>> Yes.
> 
>> But the reason that it fails is because we use getopt_long(...,
>> "+f:s:w") - where the leading '+' specifically requests that we NOT
>> allow option reordering.  Why? Because 'seq' is MOST useful if it can
>> parse negative numbers easily.  We deemed it more important to support
>> 'seq 2 -1 1' without requiring the user to write 'seq -- 2 -1 1' - but
>> in doing so, it also means that we can't reorder options, so any obvious
>> non-option (like '1' in your example) makes all other parameters
>> non-options (including '--help' in your example).
> 
>> It might be possible to do a two-pass parse over argv: one that looks
>> just for --help (and where treating -1 as an option is a no-op), and the
>> second that actually parses things in order now that it knows --help is
>> not present.  But that's a lot of code to add for a corner case, so I
>> won't be writing the patch; but I also won't turn it down if someone
>> else wants to write the patch.
> 
> Hello,
> 
> I've taken a stab at fixing this problem because it affects me fairly often.
> 
> Instead of using a two-pass system, I check if any of the args we scan are 
> --help or --version and bail if we see either.  See attached patch.
> 
> The bad side of this approach is that the -f, -s, and -w options and their 
> associated long options aren't handled so `seq 1 -w 2 10` still shows an 
> error.  Also, it's a kludgy sort of fix, so I completely understand why you 
> wouldn't want to include it.  But at least it's a step in the right direction 
> to give help when we can instead of an error.
> 
> Chad
> 

Thanks for looking at this again.
You attached the wrong patch.

A helper function to prescan argv to look for --help and call usage()
or --version and call version_etc(), while bailing itself
if it encounters '--', does seem useful to have in seq.
Though note it probably shouldn't support abbreviations like --h etc.

BTW other commands that use '+' with getopt() to exist option
processing early are tr, basename, pathchk, printf.
Though all those could interpret --option as a valid argument
and so wouldn't be appropriate to treat like this.
Likewise for all the commands the exec, like chroot, env, ...
as --options are very often passed to the delegate commands.

cheers,
Pádraig.

bug#30534: cp - Possible bugs when not preserving mode (explicit) and when copying special files

[Declercq Laurent]

I think that I did found at least two bugs in cp(1) command when the 
--no-preserve=mode option is involved and when copying special file. I 
describe each of them below.


1. Mode set on special files seem to be wrong:

Original file to copy: prw-rw-rw- 1 root staff 0 févr. 18 18:59 spfile
cp(1) command (run as root user): cp -r --no-preserve=mode spfile 
spfile_copy


Current result:

prwxr-xr-x 1 root staff 0 févr. 18 22:01 spfile_copy

Expected result (considering UMASK 0022):

prw-r--r-- 1 root staff 0 févr. 18 22:01 spfile_copy

The current behavior is due to the fact that mode used is 0777 while 
0666 should be used for files.


Possible fix: Differentiate directories from files in the copy_internal 
function.


2. Non-permission bits are preserved, even when the --no-preserve=mode 
option is involved.


Original file to copy: prwSrw-rw- 1 root staff 0 févr. 18 18:59 spfile
cp(1) command (run as root user): cp -r --no-preserve=mode spfile 
spfile_copy


Current result:

prwsr-xr-x 1 root staff 0 févr. 18 22:05 spfile_copy

Expected result (considering UMASK 0022 and without the first bug above):

prw-r--r-- 1 root staff 0 févr. 18 22:05 spfile_copy

Possible solution: Clear-out non-permission bits before calling mknod() 
and similar


Environment:
Linux jessie64 3.16.0-5-amd64 #1 SMP Debian 3.16.51-3+deb8u1 
(2018-01-08) x86_64 GNU/Linux

Checked against latest coreutils version.

Re: [SUGGESTION] nohup_${PID}.out

[Kamil Dudka]

On Monday, February 19, 2018 12:30:33 AM CET Boruch Baum wrote:
> On 2018-02-19 00:14, Bernhard Voelker wrote:
> > Have a nice day,
> > Berny
> 
> Thank you for the kind wishes. I hope the list will continue to consider
> the entirety of the suggestions in my post.

As far as I can tell, all your suggestions can be implemented as a shell 
script that uses the current implementation of nohup.  Have you tried it?

Kamil

Re: [SUGGESTION] nohup_${PID}.out

[Bernhard Voelker]

On 02/19/2018 12:30 AM, Boruch Baum wrote:

I hope the list will continue to consider
the entirety of the suggestions in my post.


What exactly do you refer to?  I think the other suggestions
seem to be based on knowing the PID, so given that this doesn't change
due to execve(), a workaround seems to be possible already, e.g.
a custom output logfile name "nohup.PID.out":

  exec nohup some/prg > nohup.$$.out

Regarding an option writing the PID: well, what about all the other
utilities executing other processes like env, nice, time, ionice,
xargs, ...?
It sounds like to become quite some bloat to the code of all the utilities
of this class - especially as the PID does not change due to execve().

Have a nice day,
Berny