Bug 107674 - [patch] sh(1): "type /NONEXISTENT" returns success
Summary: [patch] sh(1): "type /NONEXISTENT" returns success
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: Stefan Farfeleder
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-08 11:00 UTC by Martin Kammerhofer
Modified: 2007-02-04 11:10 UTC (History)
0 users

See Also:


Attachments
file.diff (649 bytes, patch)
2007-01-08 11:00 UTC, Martin Kammerhofer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Kammerhofer 2007-01-08 11:00:29 UTC
The type shell builtin indicates how each argument would be
interpreted if used as a command name. Shell scripts frequently
redirect the output to /dev/null and take a zero exit code as
confirmation that a command is available. (Similar to "test -x" which
only works with a single path name argument.)

Our sh(1) "forgets" to set the return code ($?) when testing a path
name argument, i.e. something containing the slash (/)
character. Therefore all such tests succeed unconditionally!

This e.g. breaks the hgmerge script installed with the devel/mercurial
port (when there are merge conflicts).

How-To-Repeat: $ /bin/sh -c "type /foo/bar && echo WE HAVE FOOBAR"
/foo/bar: No such file or directory
WE HAVE FOOBAR
$ /bin/sh -c "type /* && echo WE HAVE MANY COMMANDS IN /"
Comment 1 Remko Lodder freebsd_committer freebsd_triage 2007-01-08 19:24:52 UTC
Responsible Changed
From-To: freebsd-bugs->stefanf

Hello Stefan, this looks like something for you, can you have alook at 
this please?
Comment 2 dfilter service freebsd_committer freebsd_triage 2007-01-11 00:19:08 UTC
stefanf     2007-01-11 00:19:00 UTC

  FreeBSD src repository

  Modified files:
    bin/sh               exec.c 
  Log:
  Return an error status (127) from the builtins 'type' and 'command' (with
  either -v or -V) if a file with a slash in the name doesn't exist (if there is
  no slash we already did that).
  
  Additionally, suppress the error message for command -v for files with a slash.
  
  PR:             107674
  Submitted by:   Martin Kammerhofer
  
  Revision  Changes    Path
  1.30      +5 -2      src/bin/sh/exec.c
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
Comment 3 Stefan Farfeleder freebsd_committer freebsd_triage 2007-01-11 00:25:49 UTC
State Changed
From-To: open->patched

I just checked in a similar patch to current.  Can you please tell me why you 
changed access() to eaccess()?
Comment 4 mkamm 2007-01-12 17:41:54 UTC
The access(2) => eacess(2) change is for the "type" builtin
what revision 1.43 of src/bin/test/test.c was for "test".
It makes only a difference in the case of a setuid or setgid
shell.

The semantics of type are roughly "What command (if any) is this?".
Since execve(2) checks file access permissions against the effective
uid/gid, eaccess semantics are better than checking against the
real uid/gid with access(2).
Most of the sh code predates the availability of eaccess(2), I guess
that's why the original author did not use it in the first place.
The access(2) system call is broken by design and - according to the
man page - "should never be used"! (Although there is/was no security
problem in the context of the shell - 

Shorter version: Testing with eaccess gives results more consistent
with execve.

The s/access/eacess/ patch line should have made it into its own PR
since it is orthogonal (i.e. unrelated) to the subject of this one.


-- 
Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! 
Ideal für Modem und ISDN: http://www.gmx.net/de/go/smartsurfer
Comment 5 Stefan Farfeleder freebsd_committer freebsd_triage 2007-02-04 11:10:40 UTC
State Changed
From-To: patched->closed

Merged to RELENG_6.  Thanks.