[Toybox] [PATCH 1/2] ls -l: fix a bug when, without -n, both uid and gid are unavailable.

[Rob Landley]

On 08/15/2012 10:56 PM, Avery Pennarun wrote:
> In that case, the same utoa() buffer was used twice, so it would show the
> uid number in place of the gid.

Good catch.

Sigh, this is why the -n codepath is using utoa_to_buf() with a buf[12],
but I missed a codepath. Hmmm...

> ---
>  lib/lib.c |   12 +++++++-----
>  lib/lib.h |    4 ++--
>  toys/ls.c |    6 ++++--
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/lib.c b/lib/lib.c
> index e7dc45e..dc05c57 100644
> --- a/lib/lib.c
> +++ b/lib/lib.c
> @@ -463,9 +463,10 @@ struct string_list *find_in_path(char *path, char *filename)
>  // Convert unsigned int to ascii, writing into supplied buffer.  A truncated
>  // result contains the first few digits of the result ala strncpy, and is
>  // always null terminated (unless buflen is 0).
> -void utoa_to_buf(unsigned n, char *buf, unsigned buflen)
> +char *utoa_to_buf(unsigned n, char *buf, unsigned buflen)
>  {

At about this point it starts duplicating sprintf(), which ls is already
using. (The real reason for the function was that it provided its own
buffer. As soon as that stops being the case, we should probably
leverage libc.)

>  	int i, out = 0;
> +	char *p = buf;
>  
>  	if (buflen) {
>  		for (i=1000000000; i; i/=10) {
> @@ -474,22 +475,23 @@ void utoa_to_buf(unsigned n, char *buf, unsigned buflen)
>  			if ((res || out || i == 1) && --buflen>0) {
>  				out++;
>  				n -= res*i;
> -				*buf++ = '0' + res;
> +				*p++ = '0' + res;
>  			}
>  		}
> -		*buf = 0;
> +		*p = 0;
>  	}
> +	return buf;
>  }
>  
>  // Convert signed integer to ascii, using utoa_to_buf()
> -void itoa_to_buf(int n, char *buf, unsigned buflen)
> +char *itoa_to_buf(int n, char *buf, unsigned buflen)
>  {
>  	if (buflen && n<0) {
>  		n = -n;
>  		*buf++ = '-';
>  		buflen--;
>  	}
> -	utoa_to_buf((unsigned)n, buf, buflen);
> +	return utoa_to_buf((unsigned)n, buf, buflen);
>  }
>  
>  // This static buffer is used by both utoa() and itoa(), calling either one a
> diff --git a/lib/lib.h b/lib/lib.h
> index 992d7d2..2ce74a3 100644
> --- a/lib/lib.h
> +++ b/lib/lib.h
> @@ -123,8 +123,8 @@ void xchdir(char *path);
>  void xmkpath(char *path, int mode);
>  void xsetuid(uid_t uid);
>  struct string_list *find_in_path(char *path, char *filename);
> -void utoa_to_buf(unsigned n, char *buf, unsigned buflen);
> -void itoa_to_buf(int n, char *buf, unsigned buflen);
> +char *utoa_to_buf(unsigned n, char *buf, unsigned buflen);
> +char *itoa_to_buf(int n, char *buf, unsigned buflen);
>  char *utoa(unsigned n);
>  char *itoa(int n);
>  long atolx(char *c);
> diff --git a/toys/ls.c b/toys/ls.c
> index 561b353..36b7e5a 100644
> --- a/toys/ls.c
> +++ b/toys/ls.c
> @@ -120,14 +120,16 @@ static char endtype(struct stat *st)
>  
>  static char *getusername(uid_t uid)
>  {
> +    static char buf[12];

The problem here is this is essentially a global variable with limited
accessability, meaning it's allocating 12 bytes of global space every
time toybox starts up with this command built in, even when the command
you're running isn't ls. (If I let all sorts of different commands do
this, they'd add up pretty quick.)

Toybox has two workarounds for this. The first is the DEFINE_GLOBALS()
stuff, which makes a struct containing all the globals, and then puts
all those structs in a union, which means all the commands are re-using
the same space for their globals. (We know that only one command will be
running at a time, ignoring toysh for the moment, so allocating separate
space for the globals is a waste.)

(This is one of the tricks I got pushed upstream into busybox, long ago.
But I did it here first, and I think my implementation's cleaner. :)

The other trick is providing a page sized "char toybuf[];" so every
command has a scratch buffer it doesn't have to allocate, but ls is
already using that (the first 256 bytes is padding spaces, the rest is
an array of column alignment counters for ls -C and friends).

In this case, I could add an argument so the caller passes in a buffer,
but it's a bit subtle to explain what the extra argument is _for_.
Moving it to TT means the callers don't have to care about this
implementation detail.

>      struct passwd *pw = getpwuid(uid);
> -    return pw ? pw->pw_name : utoa(uid);
> +    return pw ? pw->pw_name : utoa_to_buf(uid, buf, sizeof(buf));
>  }
>  
>  static char *getgroupname(gid_t gid)
>  {
> +    static char buf[12];
>      struct group *gr = getgrgid(gid);
> -    return gr ? gr->gr_name : utoa(gid);
> +    return gr ? gr->gr_name : utoa_to_buf(gid, buf, sizeof(buf));
>  }
>  
>  // Figure out size of printable entry fields for display indent/wrap
> 

Fixed in commit 650.

Thanks,

Rob
-- 
GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code.
Either it's "mere aggregation", or a license violation.  Pick one.