[Toybox] names_to_pid() and behavior for killall and pidof commands on android

[enh]

On Mon, Jun 8, 2015 at 7:33 AM, Nicolas Noury <nicolas.noury at gmail.com>
wrote:

> Hi,
>
> I was trying toybox in nexus 9 android M preview and saw that killall
> tries to kill "all" pid and pidof
> whatever_you_type_without_a_slash_at_the_beginning answers with a list
> containing all existing pids.
> I am not sure if it belongs here or to the android bugtracker.
>
> My findings:
>
> in lib.c line 819
>          : !strcmp(basename(cmd), basename(*curname)))
>
> call to basename in both strcmp arguments does not work as expected since
> the bionic basename  gives back always the same address for the returned
> char*
>
> I was not able to find a reliable source for the expected behavior of
> baseline concerning the return buffer address and allocation.
>

from the glibc man page:

NOTES
       There  are  two  different  versions  of basename() - the POSIX
version
       described above, and the GNU version, which one gets after

           #define _GNU_SOURCE         /* See feature_test_macros(7) */
           #include <string.h>

       The GNU version never modifies its  argument,  and  returns  the
 empty
       string  when  path has a trailing slash, and in particular also when
it
       is "/".  There is no GNU version of dirname().

       With glibc, one gets the POSIX version of basename() when <libgen.h>
is
       included, and the GNU version otherwise.

(the fact not explicitly stated here being "the POSIX version sometimes
modifies its argument".)

1) should we try to fix it here or is it up to android to fix bionic
> basename behavior?
>

bionic's "POSIX" basename had the wrong CV-qualifiers since at least
cupcake (there was no NDK before that, so it didn't matter that it was
wrong until then), so even today it never modifies its arguments and uses a
thread-local buffer for backwards compatibility (with Android; obviously
this is an incompatibility with glibc). (annoyingly, we could have fixed
that for LP64 if it had been on our list of known ABI bugs. too late now
though.)

i think it's still POSIX-compliant though:

       The basename() function may modify the string pointed to by path,
and may return a pointer
       to internal storage. The returned pointer might be invalidated or
 the  storage might  be
       overwritten by a subsequent call to basename().

i'm surprised toybox goes out of its way to get the POSIX basename; as
specified it's pretty much always too dangerous to use.

// They didn't like posix basename so they defined another function with the
// same name and if you include libgen.h it #defines basename to something
// else (where they implemented the real basename), and that define breaks
// the table entry for the basename command. They didn't make a new function
// with a different name for their new behavior because gnu.
//
// Solution: don't use their broken header, provide an inline to redirect
the
// correct name to the broken name.

char *dirname(char *path);
char *__xpg_basename (char *path);
static inline char *basename(char *path) { return __xpg_basename(path); }

wouldn't it make more sense to always use a GNU-style basename? (except in
basename(1) where you need to remove any trailing slashes.)

alternatively, there's always basename_r and malloc...


> 2) if fix should be in toybox,
> a quick&dirty fix would be to copy the first basename(cmd) result into
> cmd, since it's supposed to be smaller in length and cmd is not reused with
> the same value anymore, but it relies on the assumption that buffers are
> not overlapping, or that strcpy is able to shift to left without mangling
> buffer content. If it needs to be safer, one can always allocate a temp
> buffer...
>
> Nicolas
>
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
>


-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20150608/95041164/attachment-0002.htm>