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

[enh]

rob: preferred fix here so we can provide you with the patch you want?
(this came up independently today when an OEM was surprised to find that
toybox's "killall" is rather too literal :-) )

On Mon, Jun 8, 2015 at 4:19 PM, Nicolas Noury <nicolas.noury at gmail.com>
wrote:

> Ok,
> I also noticed GNU manpage claiming this about POSIX basename:
> These functions may return pointers to statically allocated memory which
> may be overwritten by subsequent calls. Alternatively, they may return a
> pointer to some part of path, so that the string referred to by path should
> not be modified or freed until the pointer returned by the function is no
> longer required.
>
> So we should definitely copy the output buffer before and latter reuse of
> basename to avoid any usage collision if one stick to POSIX basename. (I
> attached a patch that fixes this)
>
> Alternatively, GNU basename does not advertise his behavior, but since his
> implementation relies on strrchr, it works as intended the way it's called
> in the strcmp call.
> (I tested it too, but it needs to define __USE_GNU which might be not
> wanted here)
>
> Nicolas
>
> On Mon, Jun 8, 2015 at 7:28 PM, enh <enh at google.com> wrote:
>
>>
>>
>> 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.
>>
>
>


-- 
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/20150707/07c3550d/attachment-0002.htm>