Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()

2009-09-02 Thread Ondřej Vašík
Ernest N. Mamikonyan wrote:
[returning mailing list to CC, please keep it there]

> On Tue, 01 Sep 2009 02:16:28 -0400, Ondřej Vašík  wrote:
> > Ernest N. Mamikonyan wrote:
> >> Cp(1) doesn't correctly copy extended attributes for read-only files:
> >>
> >> touch foo
> >> setfattr -n user.key -v value foo
> >> chmod a-w foo
> >> cp --preserve=xattr foo bar
> >> cp: setting attribute `user.key' for `user.key': Permission denied
> >
> > This error message is not produced by coreutils sources, but by libattr
> > - all messages about extended attributes are generated there. I'm not
> > sure why they are trying to set attributes for source file - maybe they
> > are not and access rights for destination file are more relevant.
> > Strace/ltrace of the failure could be helpful as well.
> The problem is quite simple! Cp(1) tries to change the xattrs of a file
> with mode 0400 (see attached trace). Is opening the destination file with
> initial an mode of 0600 a security (or some other) risk? I suppose that's
> what needs to be done.

Sorry, I got confused ... it's obvious - not sure about the risk, so not
trying to fix this now. What do you think, Jim/Pádraig?

Additionally it looks like there is a leak (about 36k per file for
failing xattr preserve) in copy_reg() - as the return false; should be
changed to return_val=false; - sending patch for this... but it's not
fixing the reported issue.

> >> If one uses "cp -a" instead, it simply strips the metadata and doesn't
> >> complain.
> >
> > cp -a shouldn't complain, but the xattrs are likely not preserved in
> > this case - just error message from xattr preservation is suppressed and
> > not preserving extended attributes is not considered as error in that
> > case.
> Well, the manpage says that "cp -a" is the same as "cp -dR --preserve=all."

But the manpage also says that relevant source of information is info
documentation. And info documentation mention that little difference in
behaviour. In fact cp -a behaves like "cp -dR --preserve=all", but is
silent about the failures of preserving SELinux context and/or extended
attributes.

> PS. I forgot to mention the version; it's Coreutils 7.5.
> 
> Thanks
> Ernest N. Mamikonyan 


Greetings,
  Ondřej Vašík
execve("/bin/cp", ["cp", "--preserve=all", "foo", "bar"], [/* 16 vars */]) = 0
brk(0)  = 0x8f37000
access("/etc/ld.so.preload", R_OK)  = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY)  = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=89435, ...}) = 0
mmap2(NULL, 89435, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb7f39000
close(3)= 0
open("/lib/libattr.so.1", O_RDONLY) = 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0P\20\0\0004\0\0\0h"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=17784, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7f38000
mmap2(NULL, 20652, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xb7f32000
mmap2(0xb7f36000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x3) = 0xb7f36000
close(3)= 0
open("/lib/libc.so.6", O_RDONLY)= 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\360k\1\0004\0\0\0\244"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=1478940, ...}) = 0
mmap2(NULL, 1489192, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xb7dc6000
mprotect(0xb7f2b000, 4096, PROT_NONE)   = 0
mmap2(0xb7f2c000, 12288, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x165) = 0xb7f2c000
mmap2(0xb7f2f000, 10536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0xb7f2f000
close(3)= 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7dc5000
set_thread_area({entry_number:-1 -> 6, base_addr:0xb7dc56c0, limit:1048575, seg_32bit:1, contents:0, read_exec_only:0, limit_in_pages:1, seg_not_present:0, useable:1}) = 0
mprotect(0xb7f2c000, 8192, PROT_READ)   = 0
mprotect(0xb7f36000, 4096, PROT_READ)   = 0
mprotect(0x805e000, 4096, PROT_READ)= 0
mprotect(0xb7f6f000, 4096, PROT_READ)   = 0
munmap(0xb7f39000, 89435)   = 0
brk(0)  = 0x8f37000
brk(0x8f58000)  = 0x8f58000
open("/usr/lib/locale/locale-archive", O_RDONLY|O_LARGEFILE) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=1821680, ...}) = 0
mmap2(NULL, 1821680, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb7c08000
close(3)= 0
geteuid32() = 100
stat64("bar", 0xbf924ed4)   = -1 ENOENT (No such file or directory)
stat64("foo", {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
stat64("bar", 0xbf924cfc)   = -1 ENOENT (No such file or directory)
open("foo", O_RDONLY|O_LARGEFILE)   = 3
fstat64(3, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
open("bar", O_WRONLY|O_CREAT|O_EXCL|O_LARGEFILE, 

Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()

2009-09-02 Thread Pádraig Brady
Ondřej Vašík wrote:
> Ernest N. Mamikonyan wrote:
> [returning mailing list to CC, please keep it there]
> 
>> On Tue, 01 Sep 2009 02:16:28 -0400, Ondřej Vašík  wrote:
>>> Ernest N. Mamikonyan wrote:
 Cp(1) doesn't correctly copy extended attributes for read-only files:

 touch foo
 setfattr -n user.key -v value foo
 chmod a-w foo
 cp --preserve=xattr foo bar
 cp: setting attribute `user.key' for `user.key': Permission denied
>>> This error message is not produced by coreutils sources, but by libattr
>>> - all messages about extended attributes are generated there. I'm not
>>> sure why they are trying to set attributes for source file - maybe they
>>> are not and access rights for destination file are more relevant.
>>> Strace/ltrace of the failure could be helpful as well.
>> The problem is quite simple! Cp(1) tries to change the xattrs of a file
>> with mode 0400 (see attached trace). Is opening the destination file with
>> initial an mode of 0600 a security (or some other) risk? I suppose that's
>> what needs to be done.
> 
> Sorry, I got confused ... it's obvious - not sure about the risk, so not
> trying to fix this now. What do you think, Jim/Pádraig?

I haven't looked at it, but it seems like a bug from the description.

> Additionally it looks like there is a leak (about 36k per file for
> failing xattr preserve) in copy_reg() - as the return false; should be
> changed to return_val=false; - sending patch for this... but it's not
> fixing the reported issue.

Eek! Well spotted. That also leaks the file descriptors.
I've adjusted the logs slightly in the attached patch.

cheers,
Pádraig.
>From 7877731f6e7c59905355e71519bbee7d6eac5d32 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= 
Date: Wed, 2 Sep 2009 10:34:27 +0100
Subject: [PATCH] cp: don't leak resources for each xattr preservation failure

* src/copy.c (copy_reg): Don't exit from the function after an
unsuccessful and required preservation of extended attributes.
This resulted in leaking the copy buffer and file descriptors.
---
 NEWS   |3 +++
 src/copy.c |2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 50c40be..59270eb 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,9 @@ GNU coreutils NEWS-*- outline -*-
   cp --reflink --preserve now preserves attributes when cloning a file.
   [bug introduced in coreutils-7.5]
 
+  cp --preserve=xattr no longer leaks resources on each preservation failure.
+  [bug introduced in coreutils-7.1]
+
   dd now returns non-zero status if it encountered a write error while
   printing a summary to stderr.
   [bug introduced in coreutils-6.11]
diff --git a/src/copy.c b/src/copy.c
index d1e508d..e604ec5 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -853,7 +853,7 @@ copy_reg (char const *src_name, char const *dst_name,
   if (x->preserve_xattr && ! copy_attr_by_fd (src_name, source_desc,
   dst_name, dest_desc, x)
   && x->require_preserve_xattr)
-return false;
+return_val = false;
 
   if (x->preserve_mode || x->move_mode)
 {
-- 
1.6.2.5



Bug report

2009-09-02 Thread Gil Miller
E: type 'sudo' is not known on line 58 in sources.list /etc/apt/sources.list. 
E: The list of sources could not be read.millgi...@yahoo.com


Re: Bug report

2009-09-02 Thread Bob Proulx
Gil Miller wrote:
> E: type 'sudo' is not known on line 58 in sources.list /etc/apt/sources.list. 
> E: The list of sources could not be read.millgi...@yahoo.com
> 

You have reached the GNU Coreutils mailing list.  The GNU Coreutils
are the basic file, shell and text manipulation utilities of the GNU
Operating System.  You can learn more about GNU Coreutils here:

  http://www.gnu.org/software/coreutils/

The GNU Coreutils are part of the GNU Operating System.  You can learn
more about the GNU Project here:

  http://www.gnu.org/

But you are asking about a Debian/Ubuntu system problem.  I am sorry
but this is the wrong mailing list.  We don't know anything about
Debian or Ubuntu here.  We are unable to help you here.  (However it
is apparent from the error message that you included that line 58 of
your /etc/apt/sources.list file is incorrect.  Review of that file
should show the problem.)

If you are using Debian or Ubuntu then either the Debian users mailing
list or the Ubuntu users mailing list would be a better source of
information.

  http://www.debian.org/support

  http://www.ubuntu.com/support/communitysupport

  http://www.ubuntu.com/support/community/mailinglists

Bob




[bug #27373] sort -h performs incorrectly if in utf8 locale.

2009-09-02 Thread anonymous

URL:
  

 Summary: sort -h performs incorrectly if in utf8 locale.
 Project: GNU Core Utilities
Submitted by: None
Submitted on: Wed 02 Sep 2009 19:12:06 UTC
Category: None
Severity: 3 - Normal
  Item Group: None
  Status: None
 Privacy: Public
 Assigned to: None
 Open/Closed: Open
 Discussion Lock: Any

___

Details:

Sample shell session:
% echo $LC_ALL

% echo $LC_COLLATE 
C
% echo $LANG 
en_GB.utf8
% printf "1Mi\n1M" | sort -h
1M
1Mi
% export LANG=en_GB
% printf "1Mi\n1M" | sort -h
sort: both SI and IEC prefixes present on units

The relevant lines in my locale.gen are:
en_GB.UTF-8 UTF-8  
en_GB ISO-8859-1  

However, this appears to occurs for any utf8 locale. Coreutils 7.5. Other
tests fail in a similar manner, eg: 

% export LANG=en_GB.utf8
% printf "K\nE\nM\nZ\n" | ./sort -h 
E
K
M
Z
% export LANG="en_GB"
% printf "K\nE\nM\nZ\n" | ./sort -h
K
M
E
Z




___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/





Re: Bug report

2009-09-02 Thread Giuseppe Scrivano
Hello,

can you please tell us where is it documented to ask APT related
questions to this mailing list?  It is not the first time Ubuntu
questions are directed here and in case this documentation should be
fixed.

Thanks,
Giuseppe



Gil Miller  writes:

> E: type 'sudo' is not known on line 58 in sources.list /etc/apt/sources.list. 
> E: The list of sources could not be read.millgi...@yahoo.com




[bug #27373] sort -h performs incorrectly if in utf8 locale.

2009-09-02 Thread Pádraig Brady

Follow-up Comment #1, bug #27373 (project coreutils):

I can't reproduce this or see anything wrong with the code.

All 720 of my locales work fine:

$ for LANG in $(locale -a); do printf "KnEnMnZn" | ./sort -h | tr -d 'n';
echo; done | uniq -c
720 KMEZ

Can you give your libc version?
Could you add a printf() to the find_unit_order() function in sort.c to see
if it's called?

thanks.

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/





[PATCH] simply and fix a race in 2 tail --follow tests

2009-09-02 Thread Pádraig Brady
I had noticed these tests were a little verbose and
had meant to simplify them. Coincidentally today
I triggered a race in tail-2/pid, so the attached
patch kills two birds with the one stone.

cheers,
Pádraig.
>From ba37fb2e96334b3cc784a4387d74f726be9be98d Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= 
Date: Thu, 3 Sep 2009 00:39:17 +0100
Subject: [PATCH] tests: simplify and fix a race in 2 tail --follow tests

* tests/tail-2/pid: Use the timeout command to determine process
longevity, rather than querying /proc/$pid/status.
That was racy in any case as I presume the test was copied
from tail-2/tail-n0f and wasn't updated to handle the case
where the background process was in the R (running) state.
* tests/tail-2/wait: Likewise.
---
 tests/tail-2/pid  |   37 +++--
 tests/tail-2/wait |   93 -
 2 files changed, 19 insertions(+), 111 deletions(-)

diff --git a/tests/tail-2/pid b/tests/tail-2/pid
index 86e3d60..a797666 100755
--- a/tests/tail-2/pid
+++ b/tests/tail-2/pid
@@ -23,48 +23,21 @@ fi
 
 . $srcdir/test-lib.sh
 
-require_proc_pid_status_
-
 touch here || framework_failure
 
-
 fail=0
 
-# Use tail itself to create a background process.
-
+# Use tail itself to create a background process to monitor.
 tail -f here &
 bg_pid=$!
 
-tail -s0.1 -f here --pid=$bg_pid &
-
-pid=$!
-
-sleep 0.5
-
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) ;;
-*) echo "$0: process dead? (state=$state)" 1>&2; fail=1 ;;
-  esac
-  kill $pid
-fi
+# Ensure that tail --pid=PID does not exit when PID is alive.
+timeout 1 tail -s.1 -f here --pid=$bg_pid
+test $? = 124 || fail=1
 
+# Cleanup background process
 kill $bg_pid
 
-sleep 0.5
-
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) echo $0: process still active 1>&2; fail=1 ;;
-*) ;;
-  esac
-  kill $pid
-fi
-
 # Ensure that tail --pid=PID exits successfully when PID is dead.
 # Use an unlikely-to-be-live PID
 getlimits_
diff --git a/tests/tail-2/wait b/tests/tail-2/wait
index a902b54..eb04ac2 100755
--- a/tests/tail-2/wait
+++ b/tests/tail-2/wait
@@ -24,97 +24,32 @@ fi
 
 . $srcdir/test-lib.sh
 
-require_proc_pid_status_
-
 touch here || framework_failure
 touch k || framework_failure
-(touch unreadable && chmod a-r unreadable) || framework_failure
+{ touch unreadable && chmod a-r unreadable; } || framework_failure
+cat unreadable && framework_failure
 
 fail=0
 
-tail -s0.1 -f not_here &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) echo $0: process still active 1>&2; fail=1 ;;
-*) ;;
-  esac
-  kill $pid
-fi
+timeout 1 tail -s0.1 -f not_here
+test $? = 124 && fail=1
 
-# Check if the file is really not accessible before use it.
-if ! cat unreadable; then
-tail -s0.1 -f unreadable &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-case $state in
-S*) echo $0: process still active 1>&2; fail=1 ;;
-*) ;;
-esac
-kill $pid
-fi
-fi
-
-(tail -s0.1 -f here 2>tail.err) &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) ;;
-*)  echo $0: process died 1>&2; fail=1 ;;
-  esac
-  kill $pid
-fi
+timeout 1 tail -s0.1 -f unreadable
+test $? = 124 && fail=1
 
+timeout 1 tail -s0.1 -f here 2>tail.err
+test $? = 124 || fail=1
 
 # `tail -F' must wait in any case.
 
-(tail -s0.1 -F here 2>>tail.err) &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) ;;
-*) echo $0: process died 1>&2; fail=1 ;;
-  esac
-  kill $pid
-fi
+timeout 1 tail -s0.1 -F here 2>>tail.err
+test $? = 124 || fail=1
 
-tail -s0.1 -F unreadable &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) ;;
-*) echo $0: process died 1>&2; fail=1 ;;
-  esac
-  kill $pid
-fi
+timeout 1 tail -s0.1 -F unreadable
+test $? = 124 || fail=1
 
-tail -s0.1 -F not_here &
-pid=$!
-sleep .5
-state=$(get_process_status_ $pid)
-
-if test -n "$state"; then
-  case $state in
-S*) ;;
-*) echo $0: process died 1>&2; fail=1 ;;
-  esac
-  kill $pid
-fi
+timeout 1 tail -s0.1 -F not_here
+test $? = 124 || fail=1
 
 test -s tail.err && fail=1
 
-- 
1.6.2.5



Re: [PATCH] simply and fix a race in 2 tail --follow tests

2009-09-02 Thread Jim Meyering
Pádraig Brady wrote:
> I had noticed these tests were a little verbose and
> had meant to simplify them. Coincidentally today
> I triggered a race in tail-2/pid, so the attached
> patch kills two birds with the one stone.

Good timing.  I hit this today, too.

>>From ba37fb2e96334b3cc784a4387d74f726be9be98d Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?P=C3=A1draig=20Brady?= 
> Date: Thu, 3 Sep 2009 00:39:17 +0100
> Subject: [PATCH] tests: simplify and fix a race in 2 tail --follow tests
>
> * tests/tail-2/pid: Use the timeout command to determine process
> longevity, rather than querying /proc/$pid/status.
> That was racy in any case as I presume the test was copied

slightly clearer: s/That/The latter/

FYI, tail -f with an unchanging file is different,
now that it's based on inotify.  Before, it really was
always in the 'S' state.  Now, it wakes up periodically.
Hence this race.

> from tail-2/tail-n0f and wasn't updated to handle the case
> where the background process was in the R (running) state.
> * tests/tail-2/wait: Likewise.
> ---
>  tests/tail-2/pid  |   37 +++--
>  tests/tail-2/wait |   93 
> -
>  2 files changed, 19 insertions(+), 111 deletions(-)
>
> diff --git a/tests/tail-2/pid b/tests/tail-2/pid

Looks fine, and works for me.

Thanks!




Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()

2009-09-02 Thread Jim Meyering
Pádraig Brady wrote:
> Ondřej Vašík wrote:
>> Ernest N. Mamikonyan wrote:
>> [returning mailing list to CC, please keep it there]
>>
>>> On Tue, 01 Sep 2009 02:16:28 -0400, Ondřej Vašík  wrote:
 Ernest N. Mamikonyan wrote:
> Cp(1) doesn't correctly copy extended attributes for read-only files:
>
> touch foo
> setfattr -n user.key -v value foo
> chmod a-w foo
> cp --preserve=xattr foo bar
> cp: setting attribute `user.key' for `user.key': Permission denied
 This error message is not produced by coreutils sources, but by libattr
 - all messages about extended attributes are generated there. I'm not
 sure why they are trying to set attributes for source file - maybe they
 are not and access rights for destination file are more relevant.
 Strace/ltrace of the failure could be helpful as well.
>>> The problem is quite simple! Cp(1) tries to change the xattrs of a file
>>> with mode 0400 (see attached trace). Is opening the destination file with
>>> initial an mode of 0600 a security (or some other) risk? I suppose that's
>>> what needs to be done.
>>
>> Sorry, I got confused ... it's obvious - not sure about the risk, so not
>> trying to fix this now. What do you think, Jim/Pádraig?
>
> I haven't looked at it, but it seems like a bug from the description.
>
>> Additionally it looks like there is a leak (about 36k per file for
>> failing xattr preserve) in copy_reg() - as the return false; should be
>> changed to return_val=false; - sending patch for this... but it's not
>> fixing the reported issue.
>
> Eek! Well spotted. That also leaks the file descriptors.
> I've adjusted the logs slightly in the attached patch.
>
> cheers,
> Pádraig.
>>From 7877731f6e7c59905355e71519bbee7d6eac5d32 Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= 
> Date: Wed, 2 Sep 2009 10:34:27 +0100
> Subject: [PATCH] cp: don't leak resources for each xattr preservation failure
>
> * src/copy.c (copy_reg): Don't exit from the function after an
> unsuccessful and required preservation of extended attributes.
> This resulted in leaking the copy buffer and file descriptors.

Thanks Ernest, Ondřej, and Pádraig!

Pádraig, you're welcome to push this fix.
However, please adjust the log to
s/exit/return/ or s/exit/return early/

Also, for each regression-fix like this, I find it worthwhile
to include a pointer in the log to the commit that introduced it.  E.g.,,

  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=1762092901adf0404
  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=a977dbbe78f4dcb64
  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=493c48168490247c8

I've been including the coreutils version number, 8 bytes of the commit SHA1,
date, and "summary line", e.g., for the last one listed above,

...
The bug was introduced in coreutils-7.0 via commit 0647f3eb, 2008-06-02,
"accommodate older SELinux which lacks matchpathcon_init_prefix".




Re: [bug #27373] sort -h performs incorrectly if in utf8 locale.

2009-09-02 Thread Jim Meyering
Pádraig Brady wrote:
> Follow-up Comment #1, bug #27373 (project coreutils):
>
> I can't reproduce this or see anything wrong with the code.
>
> All 720 of my locales work fine:
>
> $ for LANG in $(locale -a); do printf "KnEnMnZn" | ./sort -h | tr -d 'n';
> echo; done | uniq -c
> 720 KMEZ

Pádraig,

The web interface ate all of your backslashes:

for LANG in $(locale -a); do printf "K\nE\nM\nZ\n" \
  | sort -h|tr -d '\n'; echo; done | uniq -c