<div dir="ltr">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 :-) )</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 4:19 PM, Nicolas Noury <span dir="ltr"><<a href="mailto:nicolas.noury@gmail.com" target="_blank">nicolas.noury@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Ok,<br></div>I also noticed GNU manpage claiming this about POSIX basename:<div>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.<div class="gmail_extra"><br></div><div class="gmail_extra">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)</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra">(I tested it too, but it needs to define __USE_GNU which might be not wanted here)</div><span class="HOEnZb"><font color="#888888"><div class="gmail_extra"><br></div><div class="gmail_extra">Nicolas</div></font></span><div><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 7:28 PM, enh <span dir="ltr"><<a href="mailto:enh@google.com" target="_blank">enh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Jun 8, 2015 at 7:33 AM, Nicolas Noury <span dir="ltr"><<a href="mailto:nicolas.noury@gmail.com" target="_blank">nicolas.noury@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>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. </div><div>I am not sure if it belongs here or to the android bugtracker. </div><div><br></div><div>My findings:</div><div><br></div><div>in lib.c line 819 </div><div> : !strcmp(basename(cmd), basename(*curname)))<br></div><div><br></div><div>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*</div><div><br></div><div>I was not able to find a reliable source for the expected behavior of baseline concerning the return buffer address and allocation.</div></div></blockquote><div><br></div></span><div>from the glibc man page:</div><div><br></div><div><div>NOTES</div><div> There are two different versions of basename() - the POSIX version</div><div> described above, and the GNU version, which one gets after</div><div><br></div><div> #define _GNU_SOURCE /* See feature_test_macros(7) */</div><div> #include <string.h></div><div><br></div><div> The GNU version never modifies its argument, and returns the empty</div><div> string when path has a trailing slash, and in particular also when it</div><div> is "/". There is no GNU version of dirname().</div><div><br></div><div> With glibc, one gets the POSIX version of basename() when <libgen.h> is</div><div> included, and the GNU version otherwise.</div></div><div> <br></div><div>(the fact not explicitly stated here being "the POSIX version sometimes modifies its argument".)</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>1) should we try to fix it here or is it up to android to fix bionic basename behavior?</div></div></blockquote><div><br></div></span><div>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.)</div><div><br></div><div>i think it's still POSIX-compliant though:</div><div><br></div><div><div> The basename() function may modify the string pointed to by path, and may return a pointer</div><div> to internal storage. The returned pointer might be invalidated or the storage<span style="white-space:pre-wrap"> </span>might be</div><div> overwritten by a subsequent call to basename().</div></div><div><br></div><div>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.</div><div><br></div><div><div>// They didn't like posix basename so they defined another function with the</div><div>// same name and if you include libgen.h it #defines basename to something</div><div>// else (where they implemented the real basename), and that define breaks</div><div>// the table entry for the basename command. They didn't make a new function</div><div>// with a different name for their new behavior because gnu.</div><div>//</div><div>// Solution: don't use their broken header, provide an inline to redirect the</div><div>// correct name to the broken name.</div><div><br></div><div>char *dirname(char *path);</div><div>char *__xpg_basename (char *path);</div><div>static inline char *basename(char *path) { return __xpg_basename(path); }</div></div><div><br></div><div>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.)</div><div><br></div><div>alternatively, there's always basename_r and malloc...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><div dir="ltr"><div>2) if fix should be in toybox, </div><div>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...</div><span><font color="#888888"><div><br></div><div>Nicolas</div></font></span></div>
<br></span>_______________________________________________<br>
Toybox mailing list<br>
<a href="mailto:Toybox@lists.landley.net" target="_blank">Toybox@lists.landley.net</a><br>
<a href="http://lists.landley.net/listinfo.cgi/toybox-landley.net" target="_blank">http://lists.landley.net/listinfo.cgi/toybox-landley.net</a><br>
<br></blockquote></div><span><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>Elliott Hughes - <a href="http://who/enh" target="_blank">http://who/enh</a> - <a href="http://jessies.org/~enh/" target="_blank">http://jessies.org/~enh/</a><br>Android native code/tools questions? Mail me/drop by/add me as a reviewer.</div>
</font></span></div></div>
</blockquote></div><br></div></div></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Elliott Hughes - <a href="http://who/enh" target="_blank">http://who/enh</a> - <a href="http://jessies.org/~enh/" target="_blank">http://jessies.org/~enh/</a><br>Android native code/tools questions? Mail me/drop by/add me as a reviewer.</div>
</div>