<div dir="ltr"><font color="#0000ff">Thanks for your comment Isaac.</font><div><font color="#0000ff"><br></font><div><font color="#0000ff">I have modified the patch by addressing to your comments.<br></font><div class="gmail_extra"><font color="#0000ff">Please find the modified patch as attachment.</font></div><div class="gmail_extra"><font color="#0000ff"><br></font></div><div class="gmail_extra"><font color="#0000ff">Thanks & Regards,</font></div><div class="gmail_extra"><font color="#0000ff">Sameer Prakash Pradhan</font></div><div class="gmail_extra"><font color="#0000ff"><br></font><div class="gmail_quote">On Wed, Mar 9, 2016 at 10:09 PM, Isaac Dunham <span dir="ltr"><<a href="mailto:ibid.ag@gmail.com" target="_blank">ibid.ag@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br>
I see a few problems, which can be summed up as follows:<br>
-the help is clunky and misplaced<br>
-the patch introduces a format-string vulnerability<br>
-the feature is not actually implemented; it skips rather than escaping<br>
<br>
On Wed, Mar 09, 2016 at 02:51:26PM +0530, Sameer Pradhan wrote:<br>
<br>
--- a/toys/posix/ls.c 2016-02-26 13:23:36.000000000 +0530<br>
+++ b/toys/posix/ls.c 2016-03-09 14:47:36.000000000 +0530<br>
@@ -2,38 +2,38 @@<br>
*<br>
* Copyright 2012 Andre Renaud <<a href="mailto:andre@bluewatersys.com">andre@bluewatersys.com</a>><br>
* Copyright 2012 Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>><br>
+ * Copyright 2015 Sameer Prakash Pradhan <<a href="mailto:sameer.p.pradhan@gmail.com">sameer.p.pradhan@gmail.com</a>><br>
*<br>
* See <a href="http://opengroup.org/onlinepubs/9699919799/utilities/ls.html" rel="noreferrer" target="_blank">http://opengroup.org/onlinepubs/9699919799/utilities/ls.html</a><br>
<br>
-USE_LS(NEWTOY(ls, USE_LS_COLOR("(color):;")"ZgoACFHLRSacdfhiklmnpqrstux1[-Cxm1][-Cxml][-Cxmo][-Cxmg][-cu][-ftS][-HL]", TOYFLAG_BIN|TOYFLAG_LOCALE))<br>
+USE_LS(NEWTOY(ls, USE_LS_COLOR("(color):;")"ZgoACFHLRSab(escape)cdfhiklmnpqrstux1[-Cxm1][-Cxml][-Cxmo][-Cxmg][-cu][-ftS][-HL]", TOYFLAG_BIN|TOYFLAG_LOCALE))<br>
<br>
config LS<br>
bool "ls"<br>
default y<br>
help<br>
- usage: ls [-ACFHLRSZacdfhiklmnpqrstux1] [directory...]<br>
-<br>
+ usage: ls [-ACFHLRSZabcdfhiklmnpqrstux1] [directory...]<br>
list files<br>
<br>
what to show:<br>
- -a all files including .hidden -c use ctime for timestamps<br>
- -d directory, not contents -i inode number<br>
- -k block sizes in kilobytes -p put a '/' after dir names<br>
- -q unprintable chars as '?' -s size (in blocks)<br>
- -u use access time for timestamps -A list all files but . and ..<br>
- -H follow command line symlinks -L follow symlinks<br>
- -R recursively list files in subdirs -F append /dir *exe @sym |FIFO<br>
- -Z security context<br>
+ -a all files including .hidden -b print C-style escapes for nongraphic characters (--escape)<br>
<br>
A few points here:<br>
1-this message is *not* going to look nice; it will wrap around.<br>
It is also simultaneously verbose and unclear (does 'nongraphic' mean<br>
that it's not a box character? No, it means it's either nonprintable or<br>
has no visible output.)<br>
2-"what to show" means the data, not the presentation.<br>
'-b' changes the presntation of non-printable characters (and ' '), so it<br>
should go under "output formats".<br>
<br>
Maybe add<br>
-b escape unprintable characters and space<br>
under output formats.<br></blockquote><div><font color="#0000ff">Applied to the current patch.</font> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Further, your re-ordering below is incorrect:<br>
'1' the number should precede all characters, and sorting it right before<br>
'l' the letter is about the worst place to put it.<br>
<br>
<br>
output formats:<br>
- -1 list one file per line -C columns (sorted vertically)<br>
- -g like -l but no owner -h human readable sizes<br>
- -l long (show full details) -m comma separated<br>
- -n like -l but numeric uid/gid -o like -l but no group<br>
- -x columns (horizontal sort)<br>
+ -C columns (sorted vertically) -g like -l but no owner<br>
+ -h human readable sizes -1 list one file per line<br>
+ -l long (show full details) -m comma separated<br>
+ -n like -l but numeric uid/gid -o like -l but no group<br>
+ -x columns (horizontal sort)<br>
<br>
sorting (default is alphabetical):<br>
- -f unsorted -r reverse -t timestamp -S size<br>
+ -f unsorted -r reverse -t timestamp -S size<br>
<br>
config LS_COLOR<br>
bool "ls --color"<br>
@@ -293,7 +293,7 @@<br>
<br>
if (-1 == dirfd) {<br>
strwidth(indir->name);<br>
- perror_msg_raw(indir->name);<br>
+ perror_msg(indir->name);<br>
<font color="#0000ff">Addressed in the current patch for vulnerability.</font></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
No, this is completely broken.<br>
indir->name, being the name on disk, is user data.<br>
It cannot be trusted.<br>
For example, try running<br>
mkdir -p test/ls && touch 'test/ls/a%s' && $LS test/ls/<br>
<br>
where "$LS" is the appropriate invocation of toybox ls.<br>
return;<br>
}<br>
@@ -474,6 +474,11 @@<br>
if (flags & FLAG_q) {<br>
char *p;<br>
for (p=sort[next]->name; *p; p++) fputc(isprint(*p) ? *p : '?', stdout);<br>
+ } else if (flags & FLAG_b){<br>
+ char *b;<br>
+<br>
+ for (b = sort[next]->name; *b; b++)<br>
+ if (isgraph(*b)) fputc(*b, stdout);<br>
<br>
And I don't see where you're escaping anything.<br>
This would have to be something like this:<br>
for (b=...) {<br>
if (isgraph(*b)) fputc(*b, stdout);<br>
else printf("\\%3hho", *b);<br>
}<br><font color="#0000ff">Addressed to the current patch.</font></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Which will print C-style octal escapes for everything, rather than<br>
printing the standard \n, \r, \t ... escapes.<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Thanks & Regards<br>Sameer Prakash Pradhan</div>
</div></div></div></div>