[Toybox] [PATCH] ls new option : b

[Isaac Dunham]

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 <andre at bluewatersys.com>
  * Copyright 2012 Rob Landley <rob at landley.net>
+ * Copyright 2015 Sameer Prakash Pradhan <sameer.p.pradhan at gmail.com>
  *
  * 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.


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);

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);
  }

Which will print C-style octal escapes for everything, rather than
printing the standard \n, \r, \t ... escapes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ls.c.patch
Type: application/octet-stream
Size: 3361 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160309/3732bb27/attachment-0002.obj>