bugThe GNU Hurd - Bugs: bug #28934, execve(path, args) should take...

 
 

bug #28934: execve(path, args) should take path as a a relative path if it doesn't contain slashes

Submitted by:  Emilio Pozuelo Monfort <pochu>
Submitted on:  Fri 19 Feb 2010 03:43:23 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Standard Compliance
Status: NonePrivacy: Public
Assigned to: NoneOriginator Name: 
Open/Closed: OpenReproducibility: Every Time
Size (loc): NonePlanned Release: None
Effort: 0.00
Wiki-like text discussion box: 

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 24 May 2010 01:18:16 PM UTC, comment #14:

I've posted my last patches on http://lists.gnu.org/archive/html/bug-hurd/2010-05/msg00108.html

Emilio Pozuelo Monfort <pochu>
Project Member
Fri 16 Apr 2010 01:08:26 PM UTC, comment #13:

Note that _hurd_exec() is exposed publicly by the glibc, used by

./utils/rpctrace.c: err = _hurd_exec (traced_task, file, argv, envp);

for instance, so we can't arbitrarily add parameters.

Samuel Thibault <sthibaul>
Project Member
Fri 16 Apr 2010 12:31:08 PM UTC, comment #12:

The patch looks good, but I have a couple of suggestions.

In __execve you cannot just call _hurd_exec_file_name unconditionally
since all exec() variants end up as a __execve(). For instance,
if there is both `/bin/foo' and `./foo', execpv("foo", { "foo", 0 })
will expand to execve("/bin/foo", { "foo", 0 }, ...), which expands to
_hurd_exec_file_name(file, { "foo", 0 }, ...), which will make the exec
server think you meant `./foo'. Instead, you should actually test with
strcmp whether FILE_NAME and ARGV[0] are one and the same.

Also I think you should have a flags argument to _hurd_exec_generic
instead of a boolean, pass along the EXEC_FILE_NAME flag, and
perhaps rename the function to _hurd_exec_flags. You could even drop
_hurd_exec_file_name all together then, though you would have to move out
the #ifdef EXEC_FILE_NAME to the callers, so perhaps its worth keeping...

Regards,
Fredrik

Carl Fredrik Hammar <hammy>
Project Member
Thu 15 Apr 2010 11:18:51 PM UTC, comment #11:

It looks like I somehow fucked the previous glibc build, since I've built it from scratch this time, and it works great. No regressions and gcc works. The patch was the same.

I've modified the patch a little bit after testing that it works following the advice from Samuel:
- Moved _hurd_exec and _hurd_exec_file_name after _hurd_exec_generic in hurdexec.c, so that I don't need a prototype for _hurd_exec_generic (that's what other files do too).
- Added / improved comments.

(file #20235)

Emilio Pozuelo Monfort <pochu>
Project Member
Mon 12 Apr 2010 08:40:13 AM UTC, comment #10:

Err the glibc patch was not the one I finally used (I attached the wrong one). Here's the one I've used. I've tested in another VM and I see the same regression.

Furthermore I've found that gcc stops working fine in a machine with a new glibc.so and /hurd/exec (building my testcase fails because gcc can't find stddef.h), but that may be a consequence of the previous regression I mentioned.

It's indeed weird that it affects ELF binaries. I've looked if I could find something weird with the flags argument but everything looks right to me.

I'll try to debug the exec server with gdb to see what's going on. If you want to try this out, you can find the exec binary and libc.so on strauss.debian.net, on /home/pochu

(file #20202)

Emilio Pozuelo Monfort <pochu>
Project Member
Mon 22 Mar 2010 09:09:02 PM UTC, comment #9:

I don't see how the regression is possible given that the changes
to the exec server should only affect shell scripts not C programs.
Also your patch to glibc doesn't actually make use of _hurd_exec_file_name,
which makes the above even more bizarre.

Did you perhaps forget to add changes to the glibc patch,
or made a mistake in testing the regression?

I only had time for a brief look; I'll give more detailed comments on
the patches later.

Carl Fredrik Hammar <hammy>
Project Member
Mon 22 Mar 2010 04:25:42 PM UTC, comment #8:

Actually there seems to be a regression:

a program like

main (int argc, char **argv)
{
printf ("%s", argv[0]);
}

put into /bin/foobar, will make
$ foobar
print ./foobar instead of "foobar", which would be expected and works fine without my patches. I've looked at the patches and the Hurd code and I couldn't find why this would happen (I haven't debugged much yet as I couldn't attach gdb to the exec server).

Emilio Pozuelo Monfort <pochu>
Project Member
Sun 21 Mar 2010 06:28:35 PM UTC, comment #7:

The attached two patches fix this by adding a new exec flag in Hurd, EXEC_FILE_NAME, that will make the exec server to not look at PATH, but just exec the name it's been passed, even if it doesn't contain slashes.

Then we add a new _hurd_exec_file_name() that will use EXEC_FILE_NAME.

I didn't think a lot about the new function name, but it would be possible to change it or to deprecate _hurd_exec and create a new one that has a flags arguments (to not have two different ones) or whatever.

I've tested the patches, they fix the bug and I haven't noticed any regression.

(file #19993, file #19994)

Emilio Pozuelo Monfort <pochu>
Project Member
Fri 26 Feb 2010 06:06:07 PM UTC, comment #6:

> > The reason argv[0] is expanded is because it is passed as an argument
> > to the interpreter, otherwise the interpreter can't find it.
>
> Unless it is a relative path, of course.


I did mean any path, relative or not. What's important is that e.g. `ps'
expands to `/bin/ps' instead of non-existent file `ps'.

> >> Actually I don't think the exec server needs to know what exec*() variant
> >> has been called, since the call itself will have called execve() with
> >> the full path by looking at PATH if needed (see glibc's posix/execvpe.c,
> >> the beginning of the do-while() loop around line 140).
> >
> > As explained above, the full path isn't sent to the exec server.
>
> But since all of exec() go are wrappers around execve(), and execve()
> opens the path passed to it without looking at $PATH, and then passes
> it to the exec server as is, we can assume the path in argv[0] is
> the same one that was used to open the port, and that it's either a
> relative or an absolute path, and we don't need to look at $PATH at
> all. (That is for the exec() cases, I don't know for other things that
> use _hurd_exec()).


Normally, argv[0] is the same as the file argument to exec*(), so an
execvp("foo") will still have argv[0] = "foo", even if it results in
a execve("/bin/foo"). But note that this is just convention as argv[0]
can be an arbitrary string...

> > I now think a better solution is to change the exec server to look in
> > the current directory for the file.
> > The question is whether this is secure, and I'd say `yes' simply
> > because the path reconstruction seems pretty haphazardous as it is.
> > While the exec server does do some tests on the file with io_identity,
> > it is pretty easy for a file to fake this information, and I suspect this
> > is the reason the exec server only tries to reconstruct the path if the
> > EXEC_SECURE flag is off. To be on the safe side the current directory
> > should be checked after $PATH.
>
> I'm not really convinced about looking in the current directory,
> although if we do, doing it after looking at $PATH sounds reasonable.


I agree that this is an unsatisfying solution, but I recommend that you do
this first before you start with a more extravagant solution. Then we'll
have fixed the problem at hand even if the better solution gets stalled.

> > A final solution might be to change the exec protocol so that exec*()
> > can pass on the files path, which seems much more robust. Or possibly
> > do the checking for #!-scripts in glibc... But you don't have to worry
> > about this, unless you want to of course. :-)
>
> Changing the exec server so that we can pass it the full path sounds
> like the best option to me. What would be the best way to do it? Maybe
> adding a new _hurd_exec_path(task, file, path, argv, envp) that avoids
> looking at $PATH?


Well, this is an extensive change. You'll need to add a path parameter
to at least the following: _hurd_exec, file_exec, exec_exec, and change
all callers.

Giving them new names, e.g. _hurd_exec_path, might be a good idea to
avoid incompatibilities, but eventually we'll want to deprecate the
original versions. I don't know how this should be handled in general;
you'll want to get a more authoritative answer from Thomas Schwinge,
or possibly Olaf Buddenhagen or Samuel Thibault.

But really, do the current directory lookup first. :-)

Regards,
Fredrik

Carl Fredrik Hammar <hammy>
Project Member
Fri 26 Feb 2010 04:10:34 PM UTC, comment #5:

> The reason argv[0] is expanded is because it is passed as an argument
> to the interpreter, otherwise the interpreter can't find it.


Unless it is a relative path, of course.

> Also, ``./foo bar'' gets me `bar' in Linux not `./bar'. Are you sure
> you got `./bar' in this case?


Sorry I was wrong. I also get bar in that case.

>> Actually I don't think the exec server needs to know what exec*() variant
>> has been called, since the call itself will have called execve() with
>> the full path by looking at PATH if needed (see glibc's posix/execvpe.c,
>> the beginning of the do-while() loop around line 140).
>
> As explained above, the full path isn't sent to the exec server.


But since all of exec() go are wrappers around execve(), and execve() opens the path passed to it without looking at $PATH, and then passes it to the exec server as is, we can assume the path in argv[0] is the same one that was used to open the port, and that it's either a relative or an absolute path, and we don't need to look at $PATH at all. (That is for the exec() cases, I don't know for other things that use _hurd_exec()).

>> Another similar option is to prepend . to $PATH in that case (and then
>> the exec server will prepend ./ to the file).
>
> This isn't a good idea either since the child process will end up with
> a different $PATH as a side-effect.


Oh right.

> I now think a better solution is to change the exec server to look in
> the current directory for the file.
> The question is whether this is secure, and I'd say `yes' simply
> because the path reconstruction seems pretty haphazardous as it is.
> While the exec server does do some tests on the file with io_identity,
> it is pretty easy for a file to fake this information, and I suspect this
> is the reason the exec server only tries to reconstruct the path if the
> EXEC_SECURE flag is off. To be on the safe side the current directory
> should be checked after $PATH.


I'm not really convinced about looking in the current directory, although if we do, doing it after looking at $PATH sounds reasonable.

> A final solution might be to change the exec protocol so that exec*()
> can pass on the files path, which seems much more robust. Or possibly
> do the checking for #!-scripts in glibc... But you don't have to worry
> about this, unless you want to of course. :-)


Changing the exec server so that we can pass it the full path sounds like the best option to me. What would be the best way to do it? Maybe adding a new _hurd_exec_path(task, file, path, argv, envp) that avoids looking at $PATH?

Emilio Pozuelo Monfort <pochu>
Project Member
Fri 26 Feb 2010 03:31:57 PM UTC, comment #4:

I researched further how argv[0] is supposed to be handled and have a
much firmer grasp on the situation now.

argv[0] is only expanded to path to the executed file when the executed
file is a #!-script. This means that touching argv[0] in execve()
is not acceptable because we don't know that at this point.

The reason argv[0] is expanded is because it is passed as an argument
to the interpreter, otherwise the interpreter can't find it.

Because the exec server is passed an already opened port to the
executable, it doesn't know the path used to open the executable.
It has to reconstruct it instead.

Also, ``./foo bar'' gets me `bar' in Linux not `./bar'. Are you sure
you got `./bar' in this case?

> Actually I don't think the exec server needs to know what exec*() variant
> has been called, since the call itself will have called execve() with
> the full path by looking at PATH if needed (see glibc's posix/execvpe.c,
> the beginning of the do-while() loop around line 140).


As explained above, the full path isn't sent to the exec server.

> Anyway since that may be an implementation detail and since I don't
> know if changing the exec server to stop looking at $PATH if the file
> doesn't contain an slash will break other stuff, changing execve()
> as you propose sounds like a good idea.


It turns out my idea was rubbish. :-)

> Another similar option is to prepend . to $PATH in that case (and then
> the exec server will prepend ./ to the file).


This isn't a good idea either since the child process will end up with
a different $PATH as a side-effect.

> I'm going to prepare and test a patch for one of the two options.


Sorry for pointing you in the wrong direction. :-(

I now think a better solution is to change the exec server to look in
the current directory for the file.

The question is whether this is secure, and I'd say `yes' simply
because the path reconstruction seems pretty haphazardous as it is.
While the exec server does do some tests on the file with io_identity,
it is pretty easy for a file to fake this information, and I suspect this
is the reason the exec server only tries to reconstruct the path if the
EXEC_SECURE flag is off. To be on the safe side the current directory
should be checked after $PATH.

Please do this instead.

A final solution might be to change the exec protocol so that exec*()
can pass on the files path, which seems much more robust. Or possibly
do the checking for #!-scripts in glibc... But you don't have to worry
about this, unless you want to of course. :-)

Regards,
Fredrik

Carl Fredrik Hammar <hammy>
Project Member
Fri 26 Feb 2010 11:37:44 AM UTC, comment #3:

Hi Carl,

Actually I don't think the exec server needs to know what exec*() variant has been called, since the call itself will have called execve() with the full path by looking at PATH if needed (see glibc's posix/execvpe.c, the beginning of the do-while() loop around line 140).

Anyway since that may be an implementation detail and since I don't know if changing the exec server to stop looking at $PATH if the file doesn't contain an slash will break other stuff, changing execve() as you propose sounds like a good idea.

Another similar option is to prepend . to $PATH in that case (and then the exec server will prepend ./ to the file).

I'm going to prepare and test a patch for one of the two options.

Emilio Pozuelo Monfort <pochu>
Project Member
Wed 24 Feb 2010 01:43:59 PM UTC, comment #2:

Hi Emilio,

I'm pretty sure you are correct in that execve() should not look in $PATH,
because glibc's sysdeps/mach/hurd/execve.c does a regular file lookup.
As you say the exec server gets confused since it does not find bar in
$PATH, and tries to reconstruct a `correct' path to the file.

The root problem is that the exec server can't tell which exec() variant
was called, so it doesn't know how to interpret argv[0]. The easiest
way to fix this is to prepend `./' to argv[0] if needed in execve().
This is probably the correct way to fix it as well. I'm not sure if
this should be done to all relative paths or only ones that doesn't
contain slashes. I'll leave it up to you to investigate further what
POSIX says about it and how Linux does it.

Regards,
Fredrik

Carl Fredrik Hammar <hammy>
Project Member
Fri 19 Feb 2010 03:46:11 PM UTC, comment #1:

btw the POSIX standard that makes me think path in execve() should always be taken as a path (note the difference between path and file), from http://www.opengroup.org/onlinepubs/000095399/functions/exec.html

"""
int execve(const char path, char const argv[], char *const envp[]);
int execlp(const char file, const char arg0, ... /, (char )0 */);
[...]
The argument path points to a pathname that identifies the new process image file.

The argument file is used to construct a pathname that identifies the new process image file. If the file argument contains a slash character, the file argument shall be used as the pathname for this file. Otherwise, the path prefix for this file is obtained by a search of the directories passed as the environment variable PATH (see the Base Definitions volume of IEEEĀ StdĀ 1003.1-2001, Chapter 8, Environment Variables). If this environment variable is not present, the results of the search are implementation-defined.
"""

Emilio Pozuelo Monfort <pochu>
Project Member
Fri 19 Feb 2010 03:43:23 PM UTC, original submission:

Hi,

I've been investigating the GLib test suite errors, and I've tracked them down to what looks like a glibc or a Hurd problem.

It looks like a call to execve() and co. don't take the "path" argument as a (relative) path if it doesn't contain any slashes. So a call like execve("foo", args) doesn't behave similarly to execve("./foo", args).

From my reading of POSIX, it seems to me that those system calls that have "path" as argument (and not "file" like e.g. execvp()) should take it as a path always, unlike "file", which should be understood as a path only when it contains an slash.

A test case would be like this:

hurd:/# cat bar
#!/bin/sh
echo "\$0: $0"

hurd:/# cat foo.c
#include <stdio.h>

int
main (int argc, char **argv)
{
char *arg[] = { argv[1], NULL };

execv (*arg, arg);
perror ("execv");
return 1;
}

hurd:/# ./foo ./bar
$0: ./bar

hurd:/# ./foo bar
$0: /dev/fd/3

On Linux I get ./bar in both cases (btw PATH must not contain . to reproduce the problem).

Part of the problem seems to be in check_hashbang() in hurd's exec/hashexec.c, where we have

if (strchr (name, '/') != NULL)
error = lookup (name, 0, &name_file);
else if ((error = hurd_catch_signal
(sigmask (SIGBUS) | sigmask (SIGSEGV),
(vm_address_t) envp, (vm_address_t) envp + envplen,
&search_path, SIG_ERR)))
name_file = MACH_PORT_NULL;

Here it takes name as a path only if it contains a slash, otherwise it looks for it in PATH (that's what search_path does).

Then after that it does:

if (file_name == NULL)
{
/* We can't easily find the file.
Put it in a file descriptor and pass /dev/fd/N. */

Having /dev/fd/N in $0 instead of the real file name is what is breaking libtool logic to find .libs and execute the correct binary, breaking GLib's test suite.

I'm not sure where this should be fixed, I guess in Hurd and/or in glibc. If someone can guide me a bit I'll try to cook a patch.

Emilio Pozuelo Monfort <pochu>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #20235:  glibc.patch added by pochu (4kB - application/octet-stream)
file #20202:  glibc.patch added by pochu (3kB - application/octet-stream)
file #19993:  hurd.patch added by pochu (2kB - application/octet-stream)
file #19994:  glibc.patch added by pochu (2kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by sthibaul (Posted a comment)
  • -unavailable- added by hammy (Posted a comment)
  • -unavailable- added by pochu (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 24 May 2010 01:18:16 PM UTCpochuWiki-like text discussion boxI\'ve posted my last patches on http://lists.gnu.org/archive/html/bug-hurd/2010-05/msg00108.html=>
    Mon 24 May 2010 12:17:53 PM UTCpochuWiki-like text discussion box=>I've posted my last patches on http://lists.gnu.org/archive/html/bug-hurd/2010-05/msg00108.html
    Thu 15 Apr 2010 11:18:51 PM UTCpochuAttached File-=>Added glibc.patch, #20235
    Mon 12 Apr 2010 08:40:13 AM UTCpochuAttached File-=>Added glibc.patch, #20202
    Sun 21 Mar 2010 06:28:35 PM UTCpochuAttached File-=>Added hurd.patch, #19993
      Attached File-=>Added glibc.patch, #19994

    Back to the top


    Powered by Savane 3.1-cleanup