Bug 107515 - [patch] ls(1) bug
Summary: [patch] ls(1) bug
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 6.2-PRERELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Jaakko Heinonen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-04 12:30 UTC by Ricardo Branco
Modified: 2010-03-25 16:51 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ricardo Branco 2007-01-04 12:30:11 UTC
Why is it that /bin/ls has the following behavior?

$ mkdir /tmp/a
$ chmod 000 /tmp/a
$ ls  /tmp/a/
ls: : Permission denied
$ ls /tmp/a
ls: a: Permission denied

I think the most sensible way is to use basename() and use the last component for the error message rather than just printing whatever exists after the last slash...
Comment 1 Remko Lodder freebsd_committer freebsd_triage 2007-01-04 12:36:42 UTC
State Changed
From-To: open->closed

I am sorry but this is not a bug, the behaviour is to list the contents 
of the directory if you request the leaf ( /tmp/a/ ) and to show the 
directory itself if you request that (/tmp/a). This is desired 
functionality and should not change imo. Thus closing the ticket, thanks 
for your willingness to improve FreeBSD though
Comment 2 davidt 2007-01-04 14:38:24 UTC
On Thu, 04 Jan 2007, Remko Lodder wrote:
> Synopsis: /bin/ls bug
> 
> State-Changed-From-To: open->closed
> State-Changed-By: remko
> State-Changed-When: Thu Jan 4 12:36:42 UTC 2007
> State-Changed-Why: 
> I am sorry but this is not a bug, the behaviour is to list the contents
> of the directory if you request the leaf ( /tmp/a/ ) and to show the
> directory itself if you request that (/tmp/a). This is desired
> functionality and should not change imo.

Unfortunately that is not the behaviour exhibited by ls.  Further,
that was not the problem being reported, either.

Both "ls /tmp/a/" and "ls /tmp/a" attempt to list the contents of the
directory (as shown by the "Permission Denied" error.  The directory
itself can by listed using "ls -d /tmp/a/" (or "ls -d /tmp/a").

The actual bug being reported is a different matter, however:

$ ls /tmp/a/
ls: : Permission denied
  ^^^

The name should not be blank here, it should be something like:

$ ls /tmp/a
ls: a: Permission denied
  ^^^^

-- 
David Taylor
Comment 3 Bruce Evans 2007-01-04 23:50:12 UTC
On Thu, 4 Jan 2007, David Taylor wrote:

> On Thu, 04 Jan 2007, Remko Lodder wrote:
> > I am sorry but this is not a bug, the behaviour is to list the contents
> > of the directory if you request the leaf ( /tmp/a/ ) and to show the
> > directory itself if you request that (/tmp/a). This is desired
> > functionality and should not change imo.
>
> Unfortunately that is not the behaviour exhibited by ls.  Further,
> that was not the problem being reported, either.
>
> Both "ls /tmp/a/" and "ls /tmp/a" attempt to list the contents of the
> directory (as shown by the "Permission Denied" error.  The directory
> itself can by listed using "ls -d /tmp/a/" (or "ls -d /tmp/a").
>
> The actual bug being reported is a different matter, however:
>
> $ ls /tmp/a/
> ls: : Permission denied
>   ^^^
>
> The name should not be blank here, it should be something like:
>
> $ ls /tmp/a
> ls: a: Permission denied
>   ^^^^

Both of these behaviours are bugs.  The pathname with the problem is
/tmp/a or /tmp/a/.  ls should be reporting that.  Instead, ls apparently
does extra work to mess up the pathname, and gets it completely wrong
for the one with the trailing slash.  Truss output:

ls /tmp/a:
%%%
...
stat("/tmp/a",0xbfbfe230)			 = 0 (0x0)
open(".",0x0,00)				 = 3 (0x3)
fchdir(0x3)					 = 0 (0x0)
stat("/tmp/a",0xbfbfe1e0)			 = 0 (0x0)
open("/tmp/a",0x4,00)				 ERR#13 'Permission denied'
stat("/tmp/a",0xbfbfe1e0)			 = 0 (0x0)
open("/tmp/a",0x4,00)				 ERR#13 'Permission denied'
ls: write(2,0xbfbfdb70,4)				 = 4 (0x4)
a: Permission deniedwrite(2,0xbfbfdb90,20)				 = 20 (0x14)
%%%

ls /tmp/a/
%%%
...
stat("/tmp/a/",0xbfbfe230)			 = 0 (0x0)
open(".",0x0,00)				 = 3 (0x3)
fchdir(0x3)					 = 0 (0x0)
stat("/tmp/a/",0xbfbfe1e0)			 = 0 (0x0)
open("/tmp/a/",0x4,00)				 ERR#13 'Permission denied'
stat("/tmp/a/",0xbfbfe1e0)			 = 0 (0x0)
open("/tmp/a/",0x4,00)				 ERR#13 'Permission denied'
ls: write(2,0xbfbfdb70,4)				 = 4 (0x4)
: Permission deniedwrite(2,0xbfbfdb90,19)				 = 19 (0x13)
%%%

It works with the full pathname throughout, then apparently tries to
print the basename.  basename(1) and presumably basename(3) give a
basename of "a" for both, but ls gets the trailing slash stripping
wrong and prints "" for /tmp/a/.

It may be wrong for basename(3) to strip the slash too (since stripping
the slash is wrong if the path is a symlink), but this is the historical
behaviour and POSIX requries it.  Thus utilities should be careful about
using basename(3) if there might be a symlink involved.  ls's bug with
a symlinked variant of the unreadable directory is a little different:

Setup:
%%%
d---------  2 bde  wheel  512 Jan  5 09:41 a
lrwxr-xr-x  1 bde  wheel    1 Jan  5 10:08 b -> a
%%%

ls /tmp/b:
%%%
...
open("/tmp/b",0x4,00)				 ERR#13 'Permission denied'
ls: write(2,0xbfbfdb70,4)				 = 4 (0x4)
b: Permission deniedwrite(2,0xbfbfdb90,20)				 = 20 (0x14)
%%%

Now "b" is perfectly readable, but what it points to isn't.  It is hard for
ls to print anything better than /tmp/b in the error message, since it
only tried to access the latter.

ls /tmp/b/:
%%%
...
open("/tmp/b/",0x4,00)				 ERR#13 'Permission denied'
ls: write(2,0xbfbfdb70,4)				 = 4 (0x4)
: Permission deniedwrite(2,0xbfbfdb90,19)				 = 19 (0x13)
%%%

Same bug as for ls /tmp/b/.  Even more confusing now.

The case of ls -R broken in a slightly different way:

ls -R /tmp (where /tmp contains "a" and "b" as above):
%%%
...
open("a",0x4,027757761130)			 ERR#13 'Permission denied'
ls: write(2,0xbfbfdb70,4)				 = 4 (0x4)
a: Permission deniedwrite(2,0xbfbfdb90,20)				 = 20 (0x14)
%%%

As usual, ls cannot open "a", and it prints only "a" in the error message,
but the full path is much more needed for recursive listings since it might
be anywhere in the tree.

Incomplete fix (ls isn't actually doing extra work to get this wrong):
%%%
Index: ls.c
===================================================================
RCS file: /home/ncvs/src/bin/ls/ls.c,v
retrieving revision 1.78
diff -u -2 -r1.78 ls.c
--- ls.c	8 Jun 2004 09:30:10 -0000	1.78
+++ ls.c	4 Jan 2007 23:30:52 -0000
@@ -477,5 +477,5 @@
  		case FTS_DNR:
  		case FTS_ERR:
-			warnx("%s: %s", p->fts_name, strerror(p->fts_errno));
+			warnx("%s: %s", p->fts_path, strerror(p->fts_errno));
  			rval = 1;
  			break;
%%%

This prints the full path in the error message for the unreadable directory
(FTS_DNR) case and in some other cases.

Other things to fix:
- ls prints only fts_name in many other messages.  I think this is only
   sometimes correct.  E.g., it is normal the output format for ls -R.
   (However I rarely use ls -R since this format is hard to unimprove on.)
- I think it is a bug in fts(3) for fts_path to be empty.  The empty
   pathname is never valid, and the error is for the directory.  Apparently,
   fts(3) reduces /tmp/a/ to the invalid basename "", but does the right
   thing for /tmp/a.

Bruce
Comment 4 Remko Lodder freebsd_committer freebsd_triage 2007-01-05 06:35:44 UTC
State Changed
From-To: closed->open

I was wrong in the assumption that this is desired behaviour, 
Bruce has posted a nice explanation for this. Reopen the 
ticket.
Comment 5 Ighighi 2007-06-23 09:13:19 UTC
Bruce Evans wrote:
>  I think it is a bug in fts(3) for fts_path to be empty.  The empty
>  pathname is never valid, and the error is for the directory.  Apparently,
>  fts(3) reduces /tmp/a/ to the invalid basename "", but does the right
>  thing for /tmp/a.

Well, there's a 2 years old patch in OpenBSD applied in the same place
as yours, titled
"use fts_path if fts_name is not available", so it seems there are
cases where fts_name
could be "", the code in fts.c is theirs after all.  Check:

http://www.openbsd.org/cgi-bin/cvsweb/src/bin/ls/ls.c.diff?r1=1.22&r2=1.23
http://www.openbsd.org/cgi-bin/cvsweb/src/bin/ls/ls.c

Maybe it's time to patch this one.
Comment 6 Jaakko Heinonen freebsd_committer freebsd_triage 2010-01-08 08:05:08 UTC
Responsible Changed
From-To: freebsd-bugs->jh

Take.
Comment 7 dfilter service freebsd_committer freebsd_triage 2010-01-24 19:17:45 UTC
Author: jh
Date: Sun Jan 24 19:17:35 2010
New Revision: 202944
URL: http://svn.freebsd.org/changeset/base/202944

Log:
  Print full path in the error message. It's possible that fts(3)
  provides an empty fts_name and reporting the full path is more
  appropriate especially with the -R option.
  
  PR:		bin/107515
  Submitted by:	bde
  Approved by:	trasz (mentor)
  MFC after:	1 week

Modified:
  head/bin/ls/ls.c

Modified: head/bin/ls/ls.c
==============================================================================
--- head/bin/ls/ls.c	Sun Jan 24 19:11:08 2010	(r202943)
+++ head/bin/ls/ls.c	Sun Jan 24 19:17:35 2010	(r202944)
@@ -508,7 +508,7 @@ traverse(int argc, char *argv[], int opt
 			break;
 		case FTS_DNR:
 		case FTS_ERR:
-			warnx("%s: %s", p->fts_name, strerror(p->fts_errno));
+			warnx("%s: %s", p->fts_path, strerror(p->fts_errno));
 			rval = 1;
 			break;
 		case FTS_D:
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 8 Jaakko Heinonen freebsd_committer freebsd_triage 2010-01-24 19:33:15 UTC
State Changed
From-To: open->patched

Patched in head (r202944).
Comment 9 dfilter service freebsd_committer freebsd_triage 2010-01-31 15:07:47 UTC
Author: jh
Date: Sun Jan 31 15:07:38 2010
New Revision: 203291
URL: http://svn.freebsd.org/changeset/base/203291

Log:
  MFC r202944:
  
  Print full path in the error message. It's possible that fts(3)
  provides an empty fts_name and reporting the full path is more
  appropriate especially with the -R option.
  
  PR:		bin/107515
  Approved by:	trasz (mentor)

Modified:
  stable/8/bin/ls/ls.c
Directory Properties:
  stable/8/bin/ls/   (props changed)

Modified: stable/8/bin/ls/ls.c
==============================================================================
--- stable/8/bin/ls/ls.c	Sun Jan 31 14:51:04 2010	(r203290)
+++ stable/8/bin/ls/ls.c	Sun Jan 31 15:07:38 2010	(r203291)
@@ -508,7 +508,7 @@ traverse(int argc, char *argv[], int opt
 			break;
 		case FTS_DNR:
 		case FTS_ERR:
-			warnx("%s: %s", p->fts_name, strerror(p->fts_errno));
+			warnx("%s: %s", p->fts_path, strerror(p->fts_errno));
 			rval = 1;
 			break;
 		case FTS_D:
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 10 dfilter service freebsd_committer freebsd_triage 2010-03-25 16:31:20 UTC
Author: jh
Date: Thu Mar 25 16:31:05 2010
New Revision: 205648
URL: http://svn.freebsd.org/changeset/base/205648

Log:
  MFC r202944:
  
  Print full path in the error message. It's possible that fts(3)
  provides an empty fts_name and reporting the full path is more
  appropriate especially with the -R option.
  
  PR:		bin/107515

Modified:
  stable/7/bin/ls/ls.c
Directory Properties:
  stable/7/bin/ls/   (props changed)

Modified: stable/7/bin/ls/ls.c
==============================================================================
--- stable/7/bin/ls/ls.c	Thu Mar 25 15:56:04 2010	(r205647)
+++ stable/7/bin/ls/ls.c	Thu Mar 25 16:31:05 2010	(r205648)
@@ -504,7 +504,7 @@ traverse(int argc, char *argv[], int opt
 			break;
 		case FTS_DNR:
 		case FTS_ERR:
-			warnx("%s: %s", p->fts_name, strerror(p->fts_errno));
+			warnx("%s: %s", p->fts_path, strerror(p->fts_errno));
 			rval = 1;
 			break;
 		case FTS_D:
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 11 Jaakko Heinonen freebsd_committer freebsd_triage 2010-03-25 16:51:29 UTC
State Changed
From-To: patched->closed

Fixed in head, stable/8 and stable/7.