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