[Toybox] [PATCH] ls new option : b

[Sameer Pradhan]

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 <ibid.ag at gmail.com> 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 <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.
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160310/d5219921/attachment-0003.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ls.c.patch
Type: application/octet-stream
Size: 2101 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160310/d5219921/attachment-0005.obj>