Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs

[Sean Anderson <seanga2@gmail.com>]

On 1/14/22 5:29 AM, Daniel Gerber wrote:
> Hi,
> 
> Thanks for this feature. I've been trying it out... (This is with lib-musl-x86_64.)
> 
> Automatic mapping works:
> 
> $ unshare --map-users=auto cat /proc/self/uid_map
>           0     100000      65536
> 
> But parsing id ranges does not:
> 
> $ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
> unshare: could not parse ID: '100000,0,65536'
> 
> Fix:
> ---
> diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
> index 443358952..52bd9702a 100644
> --- a/sys-utils/unshare.c
> +++ b/sys-utils/unshare.c
> @@ -388,7 +388,7 @@ static int uint_to_id(const char *name, size_t sz)
>   	char buf[UID_BUFSIZ];
> 
>   	mem2strcpy(buf, name, sz, sizeof(buf));
> -	return strtoul_or_err(name, _("could not parse ID"));
> +	return strtoul_or_err(buf, _("could not parse ID"));
>   }
> 
>   /**
> ---
> Then, the value passed to newuidmap is still incorrect:
> 
> $ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
> newuidmap: uid range [0-655360) -> [100000-755360) not allowed
> 
> $ unshare --map-users=100000,0,0065536 cat /proc/self/uid_map
>           0     100000      65536
> 
> The count value gets zero-padded to the right at some place I've not pinned down.

It's stack garbage. Try

diff --git i/sys-utils/unshare.c w/sys-utils/unshare.c
index 3cdd90329..5ac7af3de 100644
--- i/sys-utils/unshare.c
+++ w/sys-utils/unshare.c
@@ -385,10 +385,10 @@ struct map_range {
   */
  static int uint_to_id(const char *name, size_t sz)
  {
-       char buf[UID_BUFSIZ];
+       char buf[UID_BUFSIZ] = {0};
  
-       mem2strcpy(buf, name, sz, sizeof(buf));
-       return strtoul_or_err(name, _("could not parse ID"));
+       memcpy(buf, name, min(sz, sizeof(buf) - 1));
+       return strtoul_or_err(buf, _("could not parse ID"));
  }
  
  /**
--

(actually, I have no idea what mem2strcpy is for if it doesn't put the nul-terminator at the end of sz)

> Also, I would suggest adopting the same argument order as in /proc/<pid>/uid_map and newuidmap -- inner,outer,count.

I think this is a rather silly order. Since this is a mapping, the "natural" order is

outer -> inner

and only from the new namespace's PoV is it

inner -> outer

It certainly helped me remember things once I reversed the order...

> This doc string has it reversed:

As noted above, this is intended.

> ---
> /**
>   * struct map_range - A range of IDs to map
>   * @outer: First ID inside the namespace
>   * @inner: First ID outside the namespace
> ---
> 
> And this one has inconsistent terminology:
> ---
>   * get_map_range() - Parse a mapping range from a string
>   * @s: A string of the format upper,lower,count
>   *
>   * Parse a string of the form upper,lower,count into a new mapping range.
> ---

And here you can see that I've been reading too much of shadow's man pages :)

--Sean