On Friday 02 May 2014 11:19:01 you wrote: > Tito wrote: > > On Thursday 01 May 2014 21:30:25 Ralf Friedl wrote: > >> Tito wrote: > >>> while trying to cleanup the debianutils/which command I've spotted > >>> a minor glitch in bb's find_execable function as it is not able to handle > >>> zero lenght prefixes in the PATH variable: > >>> > >>> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html > >>> 8.3 Other Environment Variables > >>> PATH > >>> snip > >>> A zero-length prefix is a legacy feature that indicates the current > >>> working directory. It appears as two adjacent colons ( "::" ), as an > >>> initial colon preceding the rest of the list, or as a trailing colon > >>> following the rest of the list. A strictly conforming application shall > >>> use an actual pathname (such as .) to represent the current working > >>> directory in PATH . > >>> > >>> The real which command (really a script) in debianutils 4.4 does in fact > >>> handle zero-length prefixes in PATH correctly: > >>> > >>> for ELEMENT in $PATH; do > >>> if [ -z "$ELEMENT" ]; then > >>> ELEMENT=. > >>> fi > >>> > >>> So PATCH 1 actually fixes this, the size impact is huge > >>> and therefore it is dependent on CONFIG_DEKTOP: > >> Why is the size impact so huge? Why don't you use the equivalent of the > >> script, if ELEMENT empty the ELEMENT="."? > > Hi, > > because it is tricky to perform the same thing that IFS=: does. > > > >> I think the line > >> > >> if (*p == ':' && p[1] != '.') > >> is even wrong and should be > >> if (*p == ':') > > No, if you remove && p[1] != '.' it will process > > also a PATH=:: that were transformed to PATH=:.: > > in the previous pass. > Actually it should do that. "PATH=::" is equivalent to "PATH=.:.:." > If you don't remove it, it won't process PATH=:./subdir > There is a similar problem for PATH=dirwithdot.: > > >> What about this change: > >> > >> while (p) { > >> n = strchr(p, ':'); > >> if (n) > >> *n++ = '\0'; > >> + if (*p == '\0') > >> + p = "."; > >> - if (*p != '\0') { /* it's not a PATH="foo::bar" > >> situation */ > >> ... > >> - } > >> The size impact should be the assignment 'p = "."' and the constant ".", > >> about 10 bytes total, no extra processing, no extra malloc. > >> > > I also thought it was that easy in the beginning, but if I recall it > > correctly in this way it was not working. With your changes: > > > > export PATH=:/usr/local/bin:/usr/bin:/bin::/usr/local/games:/usr/games: > > debian:~/Desktop/SourceCode/busybox$ ./busybox which -a busybox > > /bin/busybox > > > > but it should show also 3 times the busybox binary in the cwd. > I don't know exactly what you did, but for me it works: > $ git diff > diff --git a/libbb/execable.c b/libbb/execable.c > index 178a00a..41540dc 100644 > --- a/libbb/execable.c > +++ b/libbb/execable.c > @@ -37,6 +37,8 @@ char* FAST_FUNC find_execable(const char *filename, > char **PATHp) > n = strchr(p, ':'); > if (n) > *n++ = '\0'; > + if (*p == '\0') > + p = "."; > if (*p != '\0') { /* it's not a PATH="foo::bar" > situation */ > p = concat_path_file(p, filename); > if (execable_file(p)) { > $ PATH=:/bin::/usr/bin: ./busybox which -a busybox > ./busybox > ./busybox > ./busybox > > To avoid a warning about "." being const, it should be written as > --- a/libbb/execable.c > +++ b/libbb/execable.c > @@ -37,14 +37,12 @@ char* FAST_FUNC find_execable(const char *filename, > char **PATHp) > n = strchr(p, ':'); > if (n) > *n++ = '\0'; > - if (*p != '\0') { /* it's not a PATH="foo::bar" situation */ > - p = concat_path_file(p, filename); > + p = concat_path_file(*p ? p : ".", filename); > if (execable_file(p)) { > *PATHp = n; > return p; > } > free(p); > - } > p = n; > } /* on loop exit p == NULL */ > return p; > > > Hi Ralf, you are totally right. I retested today on clean busybox tree and with brain powered on and it works as you said. I still wonder how I could have been so blind and I'am pretty sure it was the first thing I've tested but somehow it didn't work. Shame on me. :-( That's it about PATCH 1. There is still PATCH 2....
Ciao and thanks for your time and effort to explain me the obvious, Tito _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
