Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Pádraig Brady writes: > -human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts, > -ST_NBLOCKSIZE, output_block_size)); > +! f->stat_ok ? "?" > +: human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts, > + ST_NBLOCKSIZE, output_block_size)); Just as a style thing, it's better to parenthesize expressions that cross line boundaries, e.g.: (! f->stat_ok ? "?" : human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts, ST_NBLOCKSIZE, output_block_size))
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Pádraig Brady wrote: > Jim Meyering wrote: >> Pádraig Brady wrote: >>> Jim Meyering wrote: Thanks for the review. BTW, here's the merged version: >>> And attached one handles the `ls -Ls` case, >>> which I'll push soon. >>> >>> cheers, >>> Pádraig. >>> >From 3edaa2363db0367e8fa472c4dc5c5696537cde61 Mon Sep 17 00:00:00 2001 >>> From: =?utf-8?q?P=C3=A1draig=20Brady?= >>> Date: Tue, 29 Sep 2009 15:43:01 +0100 >>> Subject: [PATCH] ls: always print "?" for allocated size of a dereferenced >>> dangling symlink >>> >>> Previously for `ls -Ls` (but not `ls -Lsl`), we referenced >>> the st_blocks returned from the previous failed stat() call. >>> This undefined value was seen to be 0 for danglink symlinks at least. >>> * src/ls.c (print_file_name_and_frills, length_of_file_name_and_frills): >>> Don't use st_blocks if the previous stat() failed >>> * tests/ls/dangle: Add a test case >>> * NEWS: Mention the bug fix >> >> Good catch. Thanks! > > The NEWS is getting a bit verbose on this minor issue. > Unless there are objections I'll roll up the NEWS from: > > ls -Li is now consistent with ls -Lil in printing "?", not "0" as the > inode of a dangling symlink. > > ls -Li no longer relies on unspecified behavior of stat/lstat. > Before this change, "ls -Li dangling-symlink" would mistakenly > print the inode number of the symlink under some conditions. > > ls -Ls is now consistent with ls -Lsl in printing "?", not "0" as the > allocated size of a dangling symlink. > > to: > > ls -is is now consistent with ls -lis in ignoring values returned > from a failed stat/lstat. For example ls -Lis now prints "?", not "0", > for the inode number and allocated size of a dereferenced dangling symlink. Agreed. Thanks.
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Jim Meyering wrote: > Pádraig Brady wrote: >> Jim Meyering wrote: >>> Thanks for the review. >>> BTW, here's the merged version: >> And attached one handles the `ls -Ls` case, >> which I'll push soon. >> >> cheers, >> Pádraig. >> >From 3edaa2363db0367e8fa472c4dc5c5696537cde61 Mon Sep 17 00:00:00 2001 >> From: =?utf-8?q?P=C3=A1draig=20Brady?= >> Date: Tue, 29 Sep 2009 15:43:01 +0100 >> Subject: [PATCH] ls: always print "?" for allocated size of a dereferenced >> dangling symlink >> >> Previously for `ls -Ls` (but not `ls -Lsl`), we referenced >> the st_blocks returned from the previous failed stat() call. >> This undefined value was seen to be 0 for danglink symlinks at least. >> * src/ls.c (print_file_name_and_frills, length_of_file_name_and_frills): >> Don't use st_blocks if the previous stat() failed >> * tests/ls/dangle: Add a test case >> * NEWS: Mention the bug fix > > Good catch. Thanks! The NEWS is getting a bit verbose on this minor issue. Unless there are objections I'll roll up the NEWS from: ls -Li is now consistent with ls -Lil in printing "?", not "0" as the inode of a dangling symlink. ls -Li no longer relies on unspecified behavior of stat/lstat. Before this change, "ls -Li dangling-symlink" would mistakenly print the inode number of the symlink under some conditions. ls -Ls is now consistent with ls -Lsl in printing "?", not "0" as the allocated size of a dangling symlink. to: ls -is is now consistent with ls -lis in ignoring values returned from a failed stat/lstat. For example ls -Lis now prints "?", not "0", for the inode number and allocated size of a dereferenced dangling symlink. cheers, Pádraig.
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Pádraig Brady wrote: > Jim Meyering wrote: >> Thanks for the review. >> BTW, here's the merged version: > > And attached one handles the `ls -Ls` case, > which I'll push soon. > > cheers, > Pádraig. >>From 3edaa2363db0367e8fa472c4dc5c5696537cde61 Mon Sep 17 00:00:00 2001 > From: =?utf-8?q?P=C3=A1draig=20Brady?= > Date: Tue, 29 Sep 2009 15:43:01 +0100 > Subject: [PATCH] ls: always print "?" for allocated size of a dereferenced > dangling symlink > > Previously for `ls -Ls` (but not `ls -Lsl`), we referenced > the st_blocks returned from the previous failed stat() call. > This undefined value was seen to be 0 for danglink symlinks at least. > * src/ls.c (print_file_name_and_frills, length_of_file_name_and_frills): > Don't use st_blocks if the previous stat() failed > * tests/ls/dangle: Add a test case > * NEWS: Mention the bug fix Good catch. Thanks!
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Jim Meyering wrote:
>
> Thanks for the review.
> BTW, here's the merged version:
And attached one handles the `ls -Ls` case,
which I'll push soon.
cheers,
Pádraig.
>From 3edaa2363db0367e8fa472c4dc5c5696537cde61 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?=
Date: Tue, 29 Sep 2009 15:43:01 +0100
Subject: [PATCH] ls: always print "?" for allocated size of a dereferenced dangling symlink
Previously for `ls -Ls` (but not `ls -Lsl`), we referenced
the st_blocks returned from the previous failed stat() call.
This undefined value was seen to be 0 for danglink symlinks at least.
* src/ls.c (print_file_name_and_frills, length_of_file_name_and_frills):
Don't use st_blocks if the previous stat() failed
* tests/ls/dangle: Add a test case
* NEWS: Mention the bug fix
---
NEWS|3 +++
src/ls.c| 12 +++-
tests/ls/dangle | 10 --
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/NEWS b/NEWS
index f1f7347..e6a860c 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,9 @@ GNU coreutils NEWS-*- outline -*-
Before this change, "ls -Li dangling-symlink" would mistakenly
print the inode number of the symlink under some conditions.
+ ls -Ls is now consistent with ls -Lsl in printing "?", not "0" as the
+ allocated size of a dangling symlink.
+
** Portability
On Solaris 9, many commands would mistakenly treat file/ the same as
diff --git a/src/ls.c b/src/ls.c
index 801e717..30df92c 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -4016,8 +4016,9 @@ print_file_name_and_frills (const struct fileinfo *f, size_t start_col)
if (print_block_size)
printf ("%*s ", format == with_commas ? 0 : block_size_width,
-human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts,
-ST_NBLOCKSIZE, output_block_size));
+! f->stat_ok ? "?"
+: human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts,
+ ST_NBLOCKSIZE, output_block_size));
if (print_scontext)
printf ("%*s ", format == with_commas ? 0 : scontext_width, f->scontext);
@@ -4234,9 +4235,10 @@ length_of_file_name_and_frills (const struct fileinfo *f)
if (print_block_size)
len += 1 + (format == with_commas
-? strlen (human_readable (ST_NBLOCKS (f->stat), buf,
- human_output_opts, ST_NBLOCKSIZE,
- output_block_size))
+? strlen (! f->stat_ok ? "?"
+ : human_readable (ST_NBLOCKS (f->stat), buf,
+human_output_opts, ST_NBLOCKSIZE,
+output_block_size))
: block_size_width);
if (print_scontext)
diff --git a/tests/ls/dangle b/tests/ls/dangle
index 6abad92..687d3df 100755
--- a/tests/ls/dangle
+++ b/tests/ls/dangle
@@ -28,7 +28,8 @@ mkdir -p dir/sub || framework_failure
ln -s dir slink-to-dir || framework_failure
mkdir d || framework_failure
ln -s no-such d/dangle || framework_failure
-echo '? dangle' > subdir_exp || framework_failure
+printf '? dangle\n' > subdir_Li_exp || framework_failure
+printf 'total 0\n? dangle\n' > subdir_Ls_exp || framework_failure
fail=0
@@ -56,6 +57,11 @@ compare out exp || fail=1
# Ensure that ls -Li prints "?" as the inode of a dangling symlink.
rm -f out
ls -Li d > out 2>/dev/null && fail=1
-compare out subdir_exp || fail=1
+compare out subdir_Li_exp || fail=1
+
+# Ensure that ls -Ls prints "?" as the allocation of a dangling symlink.
+rm -f out
+ls -Ls d > out 2>/dev/null && fail=1
+compare out subdir_Ls_exp || fail=1
Exit $fail
--
1.6.2.5
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Eric Blake wrote:
> According to Jim Meyering on 9/29/2009 5:13 AM:
>> - /* The failed stat/lstat may have modified f->stat. Clear it,
>> - since we may use at least its st_ino member, e.g.,
>> - when trying to print the inode of dangling symlink:
>> - mkdir d; ln -s no-such d/s; ls -Li d */
>> - memset (&f->stat, 0, sizeof (f->stat));
>> + f->stat.st_ino = 22;
>
> Why 22? Is this debugging leftovers?
Thanks for the review.
BTW, here's the merged version:
>From b7aaa0da8b47f4f373d3e0876bd540986278c6e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering
Date: Tue, 29 Sep 2009 07:28:01 +0200
Subject: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* src/ls.c (format_inode): Access f->stat only if f->stat_ok is set.
* NEWS (Bug fixes): Mention it.
Improved-by: Pádraig Brady
---
NEWS |4
src/ls.c |6 +++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 68ac24b..075c0fa 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ GNU coreutils NEWS-*-
outline -*-
ls -Li is now consistent with ls -Lil in printing "?", not "0" as the
inode of a dangling symlink.
+ ls -Li no longer relies on unspecified behavior of stat/lstat.
+ Before this change, "ls -Li dangling-symlink" would mistakenly
+ print the inode number of the symlink under some conditions.
+
** Portability
On Solaris 9, many commands would mistakenly treat file/ the same as
diff --git a/src/ls.c b/src/ls.c
index c8e8abb..801e717 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3563,9 +3563,9 @@ static char *
format_inode (char *buf, size_t buflen, const struct fileinfo *f)
{
assert (INT_BUFSIZE_BOUND (uintmax_t) <= buflen);
- return (f->stat.st_ino == NOT_AN_INODE_NUMBER
- ? (char *) "?"
- : umaxtostr (f->stat.st_ino, buf));
+ return (f->stat_ok && f->stat.st_ino != NOT_AN_INODE_NUMBER
+ ? umaxtostr (f->stat.st_ino, buf)
+ : (char *) "?");
}
/* Print information about F in long format. */
--
1.6.5.rc2.177.ga9dd6
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Eric Blake wrote: > According to Jim Meyering on 9/29/2009 5:13 AM: >> - /* The failed stat/lstat may have modified f->stat. Clear it, >> - since we may use at least its st_ino member, e.g., >> - when trying to print the inode of dangling symlink: >> - mkdir d; ln -s no-such d/s; ls -Li d */ >> - memset (&f->stat, 0, sizeof (f->stat)); >> + f->stat.st_ino = 22; > > Why 22? Is this debugging leftovers? haha, yes. Thanks ;-)
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Jim Meyering on 9/29/2009 5:13 AM: > - /* The failed stat/lstat may have modified f->stat. Clear it, > - since we may use at least its st_ino member, e.g., > - when trying to print the inode of dangling symlink: > - mkdir d; ln -s no-such d/s; ls -Li d */ > - memset (&f->stat, 0, sizeof (f->stat)); > + f->stat.st_ino = 22; Why 22? Is this debugging leftovers? - -- Don't work too hard, make some time for fun as well! Eric Blake e...@byu.net -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkrB8f4ACgkQ84KuGfSFAYAh0ACgrKtuS+Eg9QMsjOQAa2LrMWAm XkQAn2rHcJzA1p22poZd0kviMH0dbKWH =hb9Q -END PGP SIGNATURE-
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Pádraig Brady wrote:
> Jim Meyering wrote:
>> Nearly missed this one.
>> ls initializes a struct stat to all NUL bytes.
>> It calls stat or lstat on a dangling symlink, and that fails.
>> ls then tests stat.st_ino.
>>
>> Sometimes, it's 0, but sometimes it's the inode of the symlink,
>> possibly depending on how the package was configured/built or
>> the environment.
>> I haven't determined precisely what makes the difference,
>> but ls (from coreutils-7.6) built some way prints 0 as the inode
>> number, and other ways, it prints the inode of the dangling symlink.
>>
>>
>>>From f7db178fdff1ebb113841035b55b103e074b5f6f Mon Sep 17 00:00:00 2001
>> From: Jim Meyering
>> Date: Tue, 29 Sep 2009 07:28:01 +0200
>> Subject: [PATCH] ls: don't use an undefined struct stat after failed
>> stat/lstat
>>
>> * src/ls.c (gobble_file): After a failed stat/lstat call,
>> clear the f->stat buffer, since the syscall may have modified it,
>> and we may need to know that stat.st_ino is zero.
>
> Well spotted. By the same token is this useful?
Well spotted yourself!
I should have remembered to use the stat_ok member.
I'll merge something like this into my patch.
Thanks!
diff --git a/src/ls.c b/src/ls.c
index dc2f86e..03ef9b0 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -2777,11 +2777,7 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
if (err != 0)
{
- /* The failed stat/lstat may have modified f->stat. Clear it,
- since we may use at least its st_ino member, e.g.,
- when trying to print the inode of dangling symlink:
- mkdir d; ln -s no-such d/s; ls -Li d */
- memset (&f->stat, 0, sizeof (f->stat));
+ f->stat.st_ino = 22;
/* Failure to stat a command line argument leads to
an exit status of 2. For other files, stat failure
@@ -3569,9 +3565,9 @@ static char *
format_inode (char *buf, size_t buflen, const struct fileinfo *f)
{
assert (INT_BUFSIZE_BOUND (uintmax_t) <= buflen);
- return (f->stat.st_ino == NOT_AN_INODE_NUMBER
- ? (char *) "?"
- : umaxtostr (f->stat.st_ino, buf));
+ return (f->stat_ok && f->stat.st_ino != NOT_AN_INODE_NUMBER
+ ? umaxtostr (f->stat.st_ino, buf)
+ : (char *) "?");
}
/* Print information about F in long format. */
Re: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Jim Meyering wrote:
> Nearly missed this one.
> ls initializes a struct stat to all NUL bytes.
> It calls stat or lstat on a dangling symlink, and that fails.
> ls then tests stat.st_ino.
>
> Sometimes, it's 0, but sometimes it's the inode of the symlink,
> possibly depending on how the package was configured/built or
> the environment.
> I haven't determined precisely what makes the difference,
> but ls (from coreutils-7.6) built some way prints 0 as the inode
> number, and other ways, it prints the inode of the dangling symlink.
>
>
>>From f7db178fdff1ebb113841035b55b103e074b5f6f Mon Sep 17 00:00:00 2001
> From: Jim Meyering
> Date: Tue, 29 Sep 2009 07:28:01 +0200
> Subject: [PATCH] ls: don't use an undefined struct stat after failed
> stat/lstat
>
> * src/ls.c (gobble_file): After a failed stat/lstat call,
> clear the f->stat buffer, since the syscall may have modified it,
> and we may need to know that stat.st_ino is zero.
Well spotted. By the same token is this useful?
diff --git a/src/ls.c b/src/ls.c
index 4531b94..fe51bb8 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -4001,8 +4001,9 @@ print_file_name_and_frills (const struct fileinfo *f,
size_t start_col)
if (print_block_size)
printf ("%*s ", format == with_commas ? 0 : block_size_width,
-human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts,
-ST_NBLOCKSIZE, output_block_size));
+! f->stat_ok ? "?"
+: human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts,
+ ST_NBLOCKSIZE, output_block_size));
if (print_scontext)
printf ("%*s ", format == with_commas ? 0 : scontext_width, f->scontext);
@@ -4219,9 +4220,10 @@ length_of_file_name_and_frills (const struct fileinfo *f)
if (print_block_size)
len += 1 + (format == with_commas
-? strlen (human_readable (ST_NBLOCKS (f->stat), buf,
- human_output_opts, ST_NBLOCKSIZE,
- output_block_size))
+? strlen (! f->stat_ok ? "?"
+ : human_readable (ST_NBLOCKS (f->stat), buf,
+human_output_opts, ST_NBLOCKSIZE,
+output_block_size))
: block_size_width);
if (print_scontext)
[PATCH] ls: don't use an undefined struct stat after failed stat/lstat
Nearly missed this one.
ls initializes a struct stat to all NUL bytes.
It calls stat or lstat on a dangling symlink, and that fails.
ls then tests stat.st_ino.
Sometimes, it's 0, but sometimes it's the inode of the symlink,
possibly depending on how the package was configured/built or
the environment.
I haven't determined precisely what makes the difference,
but ls (from coreutils-7.6) built some way prints 0 as the inode
number, and other ways, it prints the inode of the dangling symlink.
>From f7db178fdff1ebb113841035b55b103e074b5f6f Mon Sep 17 00:00:00 2001
From: Jim Meyering
Date: Tue, 29 Sep 2009 07:28:01 +0200
Subject: [PATCH] ls: don't use an undefined struct stat after failed stat/lstat
* src/ls.c (gobble_file): After a failed stat/lstat call,
clear the f->stat buffer, since the syscall may have modified it,
and we may need to know that stat.st_ino is zero.
* NEWS (Bug fixes): Mention it.
---
NEWS |4
src/ls.c |6 ++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/NEWS b/NEWS
index 1a0c847..d5270c8 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,10 @@ GNU coreutils NEWS-*-
outline -*-
ls: cannot access d/s: No such file or directory
? s
+ ls -Li no longer relies on unspecified behavior of stat/lstat.
+ Before this change, "ls -Li dangling-symlink" would mistakenly
+ print the inode number of the symlink under some conditions.
+
** Portability
On Solaris 9, many commands would mistakenly treat file/ the same as
diff --git a/src/ls.c b/src/ls.c
index c8e8abb..dc2f86e 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -2777,6 +2777,12 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
if (err != 0)
{
+ /* The failed stat/lstat may have modified f->stat. Clear it,
+ since we may use at least its st_ino member, e.g.,
+ when trying to print the inode of dangling symlink:
+ mkdir d; ln -s no-such d/s; ls -Li d */
+ memset (&f->stat, 0, sizeof (f->stat));
+
/* Failure to stat a command line argument leads to
an exit status of 2. For other files, stat failure
provokes an exit status of 1. */
--
1.6.5.rc2.177.ga9dd6
