Re: exec server and /dev/fd/N
Hi, Regarding glibc checking for the presence of file_exec_file_name in configure, Thomas said he's happy if we just let the build fail if the RPC is not present. Should we check for it or is that fine? If it's fine, I believe these patches are ready to be pushed. If we need to check for it, I have an m4 macro that will check it, although it needs a few fixes (I'm an m4 noob). Roland, these patches are needed for glib's make check (and possibly other stuff). If they are good, can you commit the glibc patch? Carl will push the Hurd ones then. Thank you, Emilio
Re: exec server and /dev/fd/N
Hi, On Mon, Jul 26, 2010 at 07:28:42PM +0200, Emilio Pozuelo Monfort wrote: > New iteration. All mentioned issues have been fixed, except for the glibc > check > for the file_exec_file_name RPC, which I don't know how to do if not using > HURD_INTERFACE_VERSION. Any suggestions are welcome. I have no idea. I guess we'd need to do better then just grep for it. A proper test would probably generate fs_U.h from fs.defs and try to compile a minimal test program that includes it and calls file_exec_file_name, or something like that. Perhaps you could ask for pointers from some actual autoconf people. Hopefully, they have some kind of support list like hurd-help. Anyway, this is the only real issue left. Though, I did manage shake out some more comment errors. ;-) > I have left the HURD_INTERFACE_VERSION patch out. We should decide whether to > remove it or whether to bump it everytime we add a new RPC. Leaving it there > but > not updating it seems nonsense to me. I agree, except that we'll probably need to keep it around while old versions of glibc are still floating around, so if we decide to remove it we should just deprecate it and remove the test in glibc. When it comes to the actual decision, as long as it is easy to check if RPCs are compatible, there is no reason to keep HURD_INTERFACE_VERSION around, IMHO. Regards, Fredrik
Re: exec server and /dev/fd/N
New iteration. All mentioned issues have been fixed, except for the glibc check for the file_exec_file_name RPC, which I don't know how to do if not using HURD_INTERFACE_VERSION. Any suggestions are welcome. I have left the HURD_INTERFACE_VERSION patch out. We should decide whether to remove it or whether to bump it everytime we add a new RPC. Leaving it there but not updating it seems nonsense to me. Regards, Emilio
Re: exec server and /dev/fd/N
Hi, On Mon, May 31, 2010 at 06:27:29PM +0200, Carl Fredrik Hammar wrote: > > Regarding version.h, I've bumped HURD_INTERFACE_VERSION in 0001 for > > exec_exec_file_name, but should it be bumped in 0002 too? > > This sounds good to me but if someone else objects you should change it. antrik recommended putting this stuff in a separate patch for now. Regards, Fredrik
Re: exec server and /dev/fd/N
Hi,
I have reviewed the patches and apart from formatting there were only
a couple of issues. Next iteration is hopefully the last.
On Thu, May 27, 2010 at 06:22:26PM +0200, Emilio Pozuelo Monfort wrote:
> On 25/05/10 21:10, Carl Fredrik Hammar wrote:
> > It would also be good if you always include a ChangeLog so I can catch
> > early errors there too, which hopefully leads to less round-trips.
>
> Writing ChangeLogs still takes me a long time, so I prefer to do it when
> the patches are close to finished so I don't need to rewrite them all
> the time, at least for big patches. If that's a big issue let me know
> and I'll write them all the time.
OK, no problem. I still think it would be a good idea though, because
after you have written it initially it is pretty easy to keep it up
to date. But you should include it in the next patches since they're
almost done.
> I've tested the following upgrade scenarios:
>
> * Upgrading only glibc (libc.so + libhurduser.so). System works fine.
> Reboot. System works fine.
>
> * Upgrading glibc (libc.so + libhurduser.so) first and then hurd (exec,
> ext2fs.static, lib{disks,triv,net}fs.so). System works fine. Reboot.
> System works fine.
>
> * Upgrading only hurd (exec, ext2fs.static, lib{disks,triv,net}fs.so).
> System works fine. Reboot. System doesn't boot. The problem is that
> the exec server dies because it needs the file_exec_file_name symbol,
> which is in libhurduser.so, lib*fs.so would have exec_exec_file_name
> unresolvable too... So it's not possible to upgrade just Hurd (which
> is fine IMHO, you need a new glibc to build the new Hurd anyway) unless
> we get into weird tricks OR we move the client side RPCs from
> libhurduser.so to a library built from Hurd.
Sounds good.
> Regarding version.h, I've bumped HURD_INTERFACE_VERSION in 0001 for
> exec_exec_file_name, but should it be bumped in 0002 too?
This sounds good to me but if someone else objects you should change it.
Regards,
Fredrik
Re: exec server and /dev/fd/N
Hi, On Wed, May 26, 2010 at 12:29:06PM +0200, Emilio Pozuelo Monfort wrote: > On 26/05/10 09:32, olafbuddenha...@gmx.net wrote: > > IIRC my major concern with this approach was that this way, the > > original task has to guess what file name will be valid in the new > > task's context... I don't remember though what my conclusion was > > regarding this being better or worse than doing the guessing in exec > > :-( > > The point is that the original task doesn't need to do any guessing, > since it knows the file name. It knows the file name in its *own* namespace; but it might not necessarily be the same in the *new* task... But as Fredrik pointed out, this is probably not a serious problem. > > just copy the new .defs to /include/hurd before building the new > > glibc, which you can then use while building the new Hurd. > > Sounds good for testing, but probably not a good idea for e.g. the > Debian packages. Eh? It's the proper way: build a new glibc package with the new .defs; and then build a new Hurd package with the new glibc. It's always been done like that. There is even a special target for installing the interface definitions in the Hurd Makefile, precisely for this purpose. -antrik-
Re: exec server and /dev/fd/N
Hi, On Wed, May 26, 2010 at 04:45:12PM +0200, Carl Fredrik Hammar wrote: > > On 26/05/10 09:32, olafbuddenha...@gmx.net wrote: > > > IIRC my major concern with this approach was that this way, the > > > original task has to guess what file name will be valid in the new > > > task's context... I don't remember though what my conclusion was > > > regarding this being better or worse than doing the guessing in > > > exec :-( > But since exec still checks that the identity ports are the same and > falls back to using a "/dev/fd/N" path (to a new FD) in that case, it > is almost impossible to end up with the wrong file. Hm, good point... A situation where the new task runs in a different name space *and* needs a "real" filename, is not terribly likely... -antrik-
Re: exec server and /dev/fd/N
Hi,
On 25/05/10 21:10, Carl Fredrik Hammar wrote:
> Some of the changes that need to be made have already been discussed on
> IRC: you should use string_t instead of data_t, empty string instead of
> NULL for RPCs, and update copyright years,
All fixed.
> The first three patches are pretty useless on their own, they all affect
> the same program, and most changes are pretty mechanical, so I think
> you might as well merge them.
I've merged them in 0001.
> You also haven't changed some calls to exec_exec and _hurd_exec() in
> Hurd and glib to _hurd_exec_file_name(), which would be appropriate if
> we're deprecating _hurd_exec().
Fixed now.
> It would also be good if you always include a ChangeLog so I can catch
> early errors there too, which hopefully leads to less round-trips.
Writing ChangeLogs still takes me a long time, so I prefer to do it when
the patches are close to finished so I don't need to rewrite them all
the time, at least for big patches. If that's a big issue let me know
and I'll write them all the time.
I've tested the following upgrade scenarios:
* Upgrading only glibc (libc.so + libhurduser.so). System works fine.
Reboot. System works fine.
* Upgrading glibc (libc.so + libhurduser.so) first and then hurd (exec,
ext2fs.static, lib{disks,triv,net}fs.so). System works fine. Reboot.
System works fine.
* Upgrading only hurd (exec, ext2fs.static, lib{disks,triv,net}fs.so).
System works fine. Reboot. System doesn't boot. The problem is that
the exec server dies because it needs the file_exec_file_name symbol,
which is in libhurduser.so, lib*fs.so would have exec_exec_file_name
unresolvable too... So it's not possible to upgrade just Hurd (which
is fine IMHO, you need a new glibc to build the new Hurd anyway) unless
we get into weird tricks OR we move the client side RPCs from
libhurduser.so to a library built from Hurd.
Regarding version.h, I've bumped HURD_INTERFACE_VERSION in 0001 for
exec_exec_file_name, but should it be bumped in 0002 too?
Cheers,
Emilio
Emilio Pozuelo Monfort (3):
Add a new exec_exec_file_name RPC
Add a file_exec_file_name RPC
Use the new _hurd_exec_file_name function
TODO |2 +-
doc/hurd.texi | 22 +-
exec/exec.c | 44 +++--
exec/hashexec.c | 41 ++--
exec/priv.h |3 +-
hurd/exec.defs| 18 -
hurd/fs.defs | 27 +++--
hurd/hurd_types.h |8 ++--
hurd/version.h|2 +-
init/init.c | 76 +---
libdiskfs/boot-start.c|2 +-
libdiskfs/file-exec.c | 70 --
libfshelp/start-translator-long.c | 23 +++
libnetfs/file-exec.c | 63 ++
libtrivfs/file-exec.c | 27 +-
trans/fakeroot.c | 57 +---
utils/fakeauth.c |6 +-
utils/login.c | 22 +++---
utils/rpctrace.c |4 +-
utils/shd.c |6 +-
20 files changed, 408 insertions(+), 115 deletions(-)
Re: exec server and /dev/fd/N
Hi, On Tue, May 25, 2010 at 09:10:57PM +0200, Carl Fredrik Hammar wrote: > On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote: > > > > diff --git a/exec/hashexec.c b/exec/hashexec.c > > index 2aa3844..7a7b329 100644 > > --- a/exec/hashexec.c > > +++ b/exec/hashexec.c > > @@ -35,6 +35,7 @@ check_hashbang (struct execdata *e, > > file_t file, > > task_t oldtask, > > int flags, > > + char *filename, > > char *argv, u_int argvlen, boolean_t argv_copy, > > char *envp, u_int envplen, boolean_t envp_copy, > > mach_port_t *dtable, u_int dtablesize, boolean_t dtable_copy, I just noticed that there is already a local variable called file_name in this function (close call!). Even if there is no conflict you should change this for clarity's sake to, say, file_name_arg or file_name_exec. (There's even a variable called name_file in there, ugh) Regards, Fredrik
Re: exec server and /dev/fd/N
On Wed, May 26, 2010 at 09:42:05AM +0200, olafbuddenha...@gmx.net wrote: > > I've commented on every hunk to make sure I looked through it all, > > which makes it a bit long but hopefully easy to follow (complain if it > > isn't). > > Well, it certainly does make it pretty redundant... I prioritized keeping context over reducing redundancy. > > > +/* Deprecated. Use exec_exec_file_name instead. */ > > > > A colon would be better: ``Deprecated: use ...''. > > Or perhaps a semi-colon, or a dash? Punctuation can be so exciting... > ;-) It is, right! ;-) Anyway, my primary issue with that was the lack of two spaces, not period versus colon. Regards, Fredrik
Re: exec server and /dev/fd/N
On Wed, May 26, 2010 at 12:29:06PM +0200, Emilio Pozuelo Monfort wrote:
> On 26/05/10 09:32, olafbuddenha...@gmx.net wrote:
> > On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote:
> >
> >> Basically the problem is that in some cases the exec server can't find
> >> the file name of the file being executed (when it's a script), and
> >> then makes a hack passing /dev/fd/N to the interpreter.
> >>
> >> I tried to fix it with heuristics such as "if the file name contains
> >> no slashes, then it's relative" and passing just a flag to the exec
> >> server to avoid needing to create new RPCs, but that's not enough,
> >> since a call like execv("foo",{"bar", NULL}) (i.e. filename !=
> >> argv[0]) should work too. So the only option is to pass filename to
> >> the exec server, and just use that.
> >
> > IIRC my major concern with this approach was that this way, the original
> > task has to guess what file name will be valid in the new task's
> > context... I don't remember though what my conclusion was regarding this
> > being better or worse than doing the guessing in exec :-(
>
> The point is that the original task doesn't need to do any guessing, since it
> knows the file name. It's the one it's used to lookup the file_t. From all the
> places that use _hurd_exec or file_exec, there's only one I haven't been able
> to
> determine filename, and that's fexecve().
It is possible for a client to open a file but send a different root
to the new task. This is a pretty contrived situation but there might
be cases we're missing. For instance, in fexecve() you could send a
"/dev/fd/N" path but if that file descriptor is O_CLOEXEC it would be
closed once the program is running.
But since exec still checks that the identity ports are the same and
falls back to using a "/dev/fd/N" path (to a new FD) in that case, it
is almost impossible to end up with the wrong file. The only case it
can happen is if the filesystem is malicious and returns someone else's
identity port, but this is possible with the current implementation as
well, and I doubt it can be exploited.
Regards,
Fredrik
Re: exec server and /dev/fd/N
Emilio Pozuelo Monfort, le Wed 26 May 2010 12:29:06 +0200, a écrit : > >> There's a little bootstrapping problem here: you can't apply the four > >> Hurd patches directly and build the whole Hurd, because lib*fs will be > >> using exec_exec_file_name, but the dynamic linker can't find it, since > >> libhurduser.so should provide it but you haven't build glibc yet. So > >> to bootstrap it (without hacks like partially building Hurd, e.g. what > >> we would need to do in the Debian packages) one would need to apply > >> all but 0004 patches to Hurd and build it, then build glibc with the > >> new Hurd installed, and then finally build Hurd with the 0004 patch. > > > > It's actually way easier than that: just copy the new .defs to > > /include/hurd before building the new glibc, which you can then use > > while building the new Hurd. > > Sounds good for testing, but probably not a good idea for e.g. the Debian > packages. It's not so rare to have to do such bootstrap for Debian packages. It's simpler that having to upload a first version with the additional .defs, etc. Samuel
Re: exec server and /dev/fd/N
Hi,
On 26/05/10 09:32, olafbuddenha...@gmx.net wrote:
> On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote:
>
>> Basically the problem is that in some cases the exec server can't find
>> the file name of the file being executed (when it's a script), and
>> then makes a hack passing /dev/fd/N to the interpreter.
>>
>> I tried to fix it with heuristics such as "if the file name contains
>> no slashes, then it's relative" and passing just a flag to the exec
>> server to avoid needing to create new RPCs, but that's not enough,
>> since a call like execv("foo",{"bar", NULL}) (i.e. filename !=
>> argv[0]) should work too. So the only option is to pass filename to
>> the exec server, and just use that.
>
> IIRC my major concern with this approach was that this way, the original
> task has to guess what file name will be valid in the new task's
> context... I don't remember though what my conclusion was regarding this
> being better or worse than doing the guessing in exec :-(
The point is that the original task doesn't need to do any guessing, since it
knows the file name. It's the one it's used to lookup the file_t. From all the
places that use _hurd_exec or file_exec, there's only one I haven't been able to
determine filename, and that's fexecve().
>> There's a little bootstrapping problem here: you can't apply the four
>> Hurd patches directly and build the whole Hurd, because lib*fs will be
>> using exec_exec_file_name, but the dynamic linker can't find it, since
>> libhurduser.so should provide it but you haven't build glibc yet. So
>> to bootstrap it (without hacks like partially building Hurd, e.g. what
>> we would need to do in the Debian packages) one would need to apply
>> all but 0004 patches to Hurd and build it, then build glibc with the
>> new Hurd installed, and then finally build Hurd with the 0004 patch.
>
> It's actually way easier than that: just copy the new .defs to
> /include/hurd before building the new glibc, which you can then use
> while building the new Hurd.
Sounds good for testing, but probably not a good idea for e.g. the Debian
packages.
Emilio
Re: exec server and /dev/fd/N
Hi, On Tue, May 25, 2010 at 09:10:57PM +0200, Carl Fredrik Hammar wrote: > On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote: > The first three patches are pretty useless on their own, they all > affect the same program, and most changes are pretty mechanical, so I > think you might as well merge them. Indeed -- while I support this kind of gradual changes in general, it's probably overdoing it a bit here :-) > I've commented on every hunk to make sure I looked through it all, > which makes it a bit long but hopefully easy to follow (complain if it > isn't). Well, it certainly does make it pretty redundant... > > +/* Deprecated. Use exec_exec_file_name instead. */ > > A colon would be better: ``Deprecated: use ...''. Or perhaps a semi-colon, or a dash? Punctuation can be so exciting... ;-) -antrik-
Re: exec server and /dev/fd/N
Hi,
On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote:
> Basically the problem is that in some cases the exec server can't find
> the file name of the file being executed (when it's a script), and
> then makes a hack passing /dev/fd/N to the interpreter.
>
> I tried to fix it with heuristics such as "if the file name contains
> no slashes, then it's relative" and passing just a flag to the exec
> server to avoid needing to create new RPCs, but that's not enough,
> since a call like execv("foo",{"bar", NULL}) (i.e. filename !=
> argv[0]) should work too. So the only option is to pass filename to
> the exec server, and just use that.
IIRC my major concern with this approach was that this way, the original
task has to guess what file name will be valid in the new task's
context... I don't remember though what my conclusion was regarding this
being better or worse than doing the guessing in exec :-(
> There's a little bootstrapping problem here: you can't apply the four
> Hurd patches directly and build the whole Hurd, because lib*fs will be
> using exec_exec_file_name, but the dynamic linker can't find it, since
> libhurduser.so should provide it but you haven't build glibc yet. So
> to bootstrap it (without hacks like partially building Hurd, e.g. what
> we would need to do in the Debian packages) one would need to apply
> all but 0004 patches to Hurd and build it, then build glibc with the
> new Hurd installed, and then finally build Hurd with the 0004 patch.
It's actually way easier than that: just copy the new .defs to
/include/hurd before building the new glibc, which you can then use
while building the new Hurd.
> I mentioned this on #hurd and antrik said that the Hurd specific parts
> of libhurduser.so should maybe be moved to Hurd.
Well, there are several levels there actually. One thing would be
splitting off libhurduser; another, splitting off the hurdish syscall
implementations alltogether. Also, they could be moved to a completely
separate library; or made part of the Hurd tree. I'm not sure which
option is preferable here -- either one would be an improvement over the
current situation IMHO...
BTW, patch series are normally sent with the patches as individual mails
in a thread -- see --thread, --cover-letter, and/or --in-reply-to
options to "git format-patch". You can send them off manually, or use
"git send-email".
-antrik-
Re: exec server and /dev/fd/N
Hi, On Wed, May 26, 2010 at 01:39:24AM +0200, Emilio Pozuelo Monfort wrote: > On 25/05/10 21:10, Carl Fredrik Hammar wrote: > >> @@ -278,7 +280,9 @@ check_hashbang (struct execdata *e, > >> else > >>name = argv; > >> > >> -if (strchr (name, '/') != NULL) > >> +if (filename) > >> + error = lookup (name = filename, 0, &name_file); > >> +else if (strchr (name, '/') != NULL) > >>error = lookup (name, 0, &name_file); > >> else if ((error = hurd_catch_signal > >>(sigmask (SIGBUS) | sigmask (SIGSEGV), > > > > Should check for "" instead of null. > > Shouldn't I check both to avoid somebody using the RPC directly and sending > NULL > in filename to crash the exec server and cause a DoS? (-: You can't pass a pointer over an RPC. The segfault happens client-side in the MIG stub. -antrik-
Re: exec server and /dev/fd/N
Hi, On Tue, May 25, 2010 at 11:03:43PM +0200, Emilio Pozuelo Monfort wrote: > > The first three patches are pretty useless on their own, they all affect > > the same program, and most changes are pretty mechanical, so I think > > you might as well merge them. > > OK. I thought about going step by step to not end with one single huge patch, > but maybe that was too much ;) It could be worth it if the changes are more complex. > > Are you familiar with TopGit? It is pretty useful when developing > > patch series since you can keep history of the development of a patch. > > It is also nice since it helps you maintain the ChangeLog if you keep > > it in .topmsg, which will later become the commit message for the patch. > > I've never used it. I'm familiar with quilt, which has no history but maybe > its > usage is somewhat similar? (I once saw a comparison between git, topgit and > quilt) I'm not sure since I haven't used quilt when developing patches. The basic idea is that you have one (almost regular) git branch per patch. TopGit then maintains the dependencies between the branches, so that when you change one branch you can update a branch that depends on it by just checking it out and do a `tg update', which is very convenient. TopGit is still a bit immature but it works. Thomas is going to use it to maintain glibc patches in its Savannah repo. (Actually, I've been meaning to check out how this project is coming along.) > > Now to the details. I've commented on every hunk to make sure I looked > > through it all, which makes it a bit long but hopefully easy to follow > > (complain if it isn't). > > I think that's perfect. > > All your comments look fine and I'll address the problems in the next patch. OK, looking forward to it. :-) Regards, Fredrik
Re: exec server and /dev/fd/N
On Wed, May 26, 2010 at 01:39:24AM +0200, Emilio Pozuelo Monfort wrote: > On 25/05/10 21:10, Carl Fredrik Hammar wrote: > >> @@ -278,7 +280,9 @@ check_hashbang (struct execdata *e, > >> else > >>name = argv; > >> > >> -if (strchr (name, '/') != NULL) > >> +if (filename) > >> + error = lookup (name = filename, 0, &name_file); > >> +else if (strchr (name, '/') != NULL) > >>error = lookup (name, 0, &name_file); > >> else if ((error = hurd_catch_signal > >>(sigmask (SIGBUS) | sigmask (SIGSEGV), > > > > Should check for "" instead of null. > > Shouldn't I check both to avoid somebody using the RPC directly and sending > NULL > in filename to crash the exec server and cause a DoS? (-: You can't actually send NULL if it's a string_t since it is a fixed-size array, but I guess it wouldn't hurt. Regards, Fredrik
Re: exec server and /dev/fd/N
On Tue, May 25, 2010 at 11:30:07PM +0200, Samuel Thibault wrote: > Emilio Pozuelo Monfort, le Tue 25 May 2010 23:03:43 +0200, a écrit : > > > Oh, I haven't noticed this file before. :-) > > > > It's the list of symbols libc.so should export. Since _hurd_exec is a public > > API, I guessed _file_name should be too. But if we don't want it to be > > public, I > > can probably put the prototype in hurd/hurdexec.h and include "hurdexec.h" > > from > > execve.c. > > I believe it should be as public as _hurd_exec. Oh, definately since it should be used instead of _hurd_exec, which is used in some places of the Hurd. I wasn't questioning whether it should be in the file or not, just in what section of the file. > > > I don't really know how this file works but this seems to be a section > > > devoted to compatibility to an older version of glibc so I doubt this is > > > the correct place to put a new function. You might want to ask Samuel > > > or Thomas about it, or if that fails, libc-alpha and Roland. > > The additional symbol needs to be added to a new GLIBC_2.foobar stanza > corresponding to the version of glibc in which it'll eventuelly get > added. Thanks for clearing this up. Regards, Fredrik
Re: exec server and /dev/fd/N
On 25/05/10 21:10, Carl Fredrik Hammar wrote: >> @@ -278,7 +280,9 @@ check_hashbang (struct execdata *e, >>else >> name = argv; >> >> - if (strchr (name, '/') != NULL) >> + if (filename) >> +error = lookup (name = filename, 0, &name_file); >> + else if (strchr (name, '/') != NULL) >> error = lookup (name, 0, &name_file); >>else if ((error = hurd_catch_signal >> (sigmask (SIGBUS) | sigmask (SIGSEGV), > > Should check for "" instead of null. Shouldn't I check both to avoid somebody using the RPC directly and sending NULL in filename to crash the exec server and cause a DoS? (-: Emilio
Re: exec server and /dev/fd/N
Emilio Pozuelo Monfort, le Tue 25 May 2010 23:03:43 +0200, a écrit : > > Oh, I haven't noticed this file before. :-) > > It's the list of symbols libc.so should export. Since _hurd_exec is a public > API, I guessed _file_name should be too. But if we don't want it to be > public, I > can probably put the prototype in hurd/hurdexec.h and include "hurdexec.h" > from > execve.c. I believe it should be as public as _hurd_exec. > > I don't really know how this file works but this seems to be a section > > devoted to compatibility to an older version of glibc so I doubt this is > > the correct place to put a new function. You might want to ask Samuel > > or Thomas about it, or if that fails, libc-alpha and Roland. The additional symbol needs to be added to a new GLIBC_2.foobar stanza corresponding to the version of glibc in which it'll eventuelly get added. Samuel
Re: exec server and /dev/fd/N
Hi Carl,
Thanks for the thorough review!
On 25/05/10 21:10, Carl Fredrik Hammar wrote:
> Some of the changes that need to be made have already been discussed on
> IRC: you should use string_t instead of data_t, empty string instead of
> NULL for RPCs, and update copyright years,
OK
> The first three patches are pretty useless on their own, they all affect
> the same program, and most changes are pretty mechanical, so I think
> you might as well merge them.
OK. I thought about going step by step to not end with one single huge patch,
but maybe that was too much ;)
> You also haven't changed some calls to exec_exec and _hurd_exec() in
> Hurd and glib to _hurd_exec_file_name(), which would be appropriate if
> we're deprecating _hurd_exec().
Will do.
> It would also be good if you always include a ChangeLog so I can catch
> early errors there too, which hopefully leads to less round-trips.
OK
> Are you familiar with TopGit? It is pretty useful when developing
> patch series since you can keep history of the development of a patch.
> It is also nice since it helps you maintain the ChangeLog if you keep
> it in .topmsg, which will later become the commit message for the patch.
I've never used it. I'm familiar with quilt, which has no history but maybe its
usage is somewhat similar? (I once saw a comparison between git, topgit and
quilt)
> Now to the details. I've commented on every hunk to make sure I looked
> through it all, which makes it a bit long but hopefully easy to follow
> (complain if it isn't).
I think that's perfect.
All your comments look fine and I'll address the problems in the next patch.
Just one comment:
>> diff --git a/hurd/Versions b/hurd/Versions
>> index 83c8ab1..81cd904 100644
>> --- a/hurd/Versions
>> +++ b/hurd/Versions
>> @@ -73,6 +73,7 @@ libc {
>> _hurd_critical_section_unlock;
>> _hurd_exception2signal;
>> _hurd_exec;
>> +_hurd_exec_file_name;
>> _hurd_fd_get;
>> _hurd_init;
>> _hurd_intern_fd;
>
> Oh, I haven't noticed this file before. :-)
>
> I don't really know how this file works but this seems to be a section
> devoted to compatibility to an older version of glibc so I doubt this is
> the correct place to put a new function. You might want to ask Samuel
> or Thomas about it, or if that fails, libc-alpha and Roland.
It's the list of symbols libc.so should export. Since _hurd_exec is a public
API, I guessed _file_name should be too. But if we don't want it to be public, I
can probably put the prototype in hurd/hurdexec.h and include "hurdexec.h" from
execve.c.
Cheers,
Emilio
Re: exec server and /dev/fd/N
Hi,
On Mon, May 24, 2010 at 12:08:10PM +0200, Emilio Pozuelo Monfort wrote:
>
> These are a series of patches to fix https://savannah.gnu.org/bugs/?28934
>
> Basically the problem is that in some cases the exec server can't find the
> file
> name of the file being executed (when it's a script), and then makes a hack
> passing /dev/fd/N to the interpreter.
>
> I tried to fix it with heuristics such as "if the file name contains no
> slashes,
> then it's relative" and passing just a flag to the exec server to avoid
> needing
> to create new RPCs, but that's not enough, since a call like
> execv("foo",{"bar",
> NULL}) (i.e. filename != argv[0]) should work too. So the only option is to
> pass
> filename to the exec server, and just use that.
>
> So this patch adds two new RPCs: exec_exec_file_name RPC and
> file_exec_file_name
> one. Then libc can use exec_exec_file_name in hurdexec.c.
Good job! There's still work to do but it's a pretty solid base.
First up, a couple of overall suggestions.
Some of the changes that need to be made have already been discussed on
IRC: you should use string_t instead of data_t, empty string instead of
NULL for RPCs, and update copyright years,
The first three patches are pretty useless on their own, they all affect
the same program, and most changes are pretty mechanical, so I think
you might as well merge them.
You also haven't changed some calls to exec_exec and _hurd_exec() in
Hurd and glib to _hurd_exec_file_name(), which would be appropriate if
we're deprecating _hurd_exec().
It would also be good if you always include a ChangeLog so I can catch
early errors there too, which hopefully leads to less round-trips.
Are you familiar with TopGit? It is pretty useful when developing
patch series since you can keep history of the development of a patch.
It is also nice since it helps you maintain the ChangeLog if you keep
it in .topmsg, which will later become the commit message for the patch.
Now to the details. I've commented on every hunk to make sure I looked
through it all, which makes it a bit long but hopefully easy to follow
(complain if it isn't).
> --- a/exec/exec.c
> +++ b/exec/exec.c
> @@ -1433,7 +1433,7 @@ do_exec (file_t file,
> {
>/* Check for a #! executable file. */
>check_hashbang (&e,
> - file, oldtask, flags,
> + file, oldtask, flags, NULL,
> argv, argvlen, argv_copy,
> envp, envplen, envp_copy,
> dtable, dtablesize, dtable_copy,
Not needed if merged with patch 2.
> diff --git a/exec/hashexec.c b/exec/hashexec.c
> index 2aa3844..7a7b329 100644
> --- a/exec/hashexec.c
> +++ b/exec/hashexec.c
> @@ -35,6 +35,7 @@ check_hashbang (struct execdata *e,
> file_t file,
> task_t oldtask,
> int flags,
> + char *filename,
> char *argv, u_int argvlen, boolean_t argv_copy,
> char *envp, u_int envplen, boolean_t envp_copy,
> mach_port_t *dtable, u_int dtablesize, boolean_t dtable_copy,
Good.
> @@ -225,7 +226,8 @@ check_hashbang (struct execdata *e,
> file_name = NULL;
> else if (! (flags & EXEC_SECURE))
> {
> - /* Try to figure out the file's name. We guess that if ARGV[0]
> + /* Try to figure out the file's name. If FILENAME is not NULL,
> + then it's the file's name. Otherwise we guess that if ARGV[0]
>contains a slash, it might be the name of the file; and that
>if it contains no slash, looking for files named by ARGV[0] in
>the `PATH' environment variable might find it. */
There should be two spaces after end of a sentence.
> @@ -278,7 +280,9 @@ check_hashbang (struct execdata *e,
> else
> name = argv;
>
> - if (strchr (name, '/') != NULL)
> + if (filename)
> + error = lookup (name = filename, 0, &name_file);
> + else if (strchr (name, '/') != NULL)
> error = lookup (name, 0, &name_file);
> else if ((error = hurd_catch_signal
> (sigmask (SIGBUS) | sigmask (SIGSEGV),
Should check for "" instead of null.
> diff --git a/exec/priv.h b/exec/priv.h
> index 7cee15e..fa41f7c 100644
> --- a/exec/priv.h
> +++ b/exec/priv.h
> @@ -171,6 +171,7 @@ void check_hashbang (struct execdata *e,
>file_t file,
>task_t oldtask,
>int flags,
> + char *filename,
>char *argv, u_int argvlen, boolean_t argv_copy,
>char *envp, u_int envplen, boolean_t envp_copy,
>mach_port_t *dtable, u_int dtablesize,
Good.
> >From 80d9b08fc69f694448d5d53dea16cf14e54d1dde Mon Sep 17 00:00:00 2001
> From: Emilio Pozuelo Monfort
> Date: Wed, 19 May 2010 23:34:12 +0200
> Subject: [PATCH 2/4] Add a filename argument to
