[Pkg-shadow-devel] [PATCH] newuidmap, newgidmap: Correct the range size sanity check in get_map_ranges

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm at xmission.com):
> Serge Hallyn <serge.hallyn at ubuntu.com> writes:
> 
> > Quoting Eric W. Biederman (ebiederm at xmission.com):
> >> 
> >> The number of ranges should be the ceiling of the number of arguments divided
> >> by three.
> >> 
> >> Without this fix newuidmap and newgidmap always report and error and fail,
> >> which is very much not what we want.
> >
> > Are you sure?
> 
> Certain enough that I tracked it down to that line.
> 
> > Without looking back at the code, I can say I've used
> > them quite a bit and never seen an error.  When I originally looked
> > at the code the -2 + 2 did annoy me, but I assumed it was supposed to
> > remind you of something :)
> 
> Yes but then you copied that test to a location where argc - 2 was
> already present.  So it is certainly a correct transformation.

Oh, I see.

> > Can you give an example command line that fails for you?
> 
> The command line I was using when this triggered was:
> 
> newuidmap $child 0 $subuid 1000 $uid $uid 1
> 
> Let me check my math to make certain the problem checks out.
> argc == 8 on entry into main.
> 
> argc - 2 (newuidmap and $child) == 6
> 
> Hmm.
> 
> You know I must have made a typo, and was short an argument when I was
> testing that.   Ugh.
> 
> So perhaps things are not quite as bad as I made out.  But the fix is
> definitely useful as otherwise we don't handle the case of being short

Right, sorry, so it is.

> an argument or two correctly.  Which should hit the next test and
> complain about being short arguments and should not complain that range
> is incorrect.
> 
> Eric
> 
> 
> >> Signed-off-by: "Eric W. Biederman" <ebiederm at xmission.com>

Acked-by: Serge Hallyn <serge.hallyn at canonical.com>

I'll push to git.  Thanks, Eric.

> >> ---
> >>  libmisc/idmapping.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
> >> index 81e85d5..714c29e 100644
> >> --- a/libmisc/idmapping.c
> >> +++ b/libmisc/idmapping.c
> >> @@ -47,7 +47,7 @@ struct map_range *get_map_ranges(int ranges, int argc, char **argv)
> >>  		return NULL;
> >>  	}
> >>  
> >> -	if (ranges != ((argc - 2) + 2) / 3) {
> >> +	if (ranges != ((argc + 2) / 3)) {
> >>  		fprintf(stderr, "%s: ranges: %u is wrong for argc: %d\n", Prog, ranges, argc);
> >>  		return NULL;
> >>  	}
> >> -- 
> >> 1.7.10.4
> >> 
> >> 
> >> _______________________________________________
> >> Pkg-shadow-devel mailing list
> >> Pkg-shadow-devel at lists.alioth.debian.org
> >> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-shadow-devel