[Pkg-shadow-devel] Newuidmap works with usernames instead of uids

[Bostjan Skufca]

Also, I  ran some benchmarks of the old and new code, under the
following conditions:
- modest i3 3.3GHz machine, fairly unloaded
- 1.000.000 entries in /etc/subuid
- command: time ./src/newuidmap 8260 0 100000 100
- command fails as there is no appropriate entry in /etc/subuid
- results repeated several times, average taken

Here are the results(n=1 000 000):
- 0.290s: Single loop, no syscalls (old behaviour)
- 0.320s: Double loop, no syscalls (new behaviour, but disabled all
syscalls in the second loop, temporarily)
- 7.500s: Double loop, with getpwnam() syscalls in second loop

This last entry should concern us, but I think that people who intend
to put 1M records into /etc/subuid, will gladly write their own tools
to optimize this.

I rerun the test, with single loop without syscalls, and varied the
number of entries in /etc/subuid. Results:
- 0.001s: with      1000 entries
- 0.004s: with    10 000 entries
- 0.032s: with   100 000 entries
- 0.290s: with 1 000 000 entries

Double loop without syscalls, and varied the number of entries in
/etc/subuid. Results:
- 0.001s: with      1000 entries
- 0.004s: with    10 000 entries
- 0.033s: with   100 000 entries
- 0.300s: with 1 000 000 entries

Obviously, number of loops does not matter all that much if no
syscalls are used. Also, second loop is WAY faster than the first one.

Last test I did was with the final patch applied as-is. Double loop,
with syscalls in the second loop. Results:
- 0.009s: with      1000 entries
- 0.075s: with    10 000 entries
- 0.740s: with   100 000 entries
- 7.500s: with 1 000 000 entries

Realistically I believe that people will max this implementation at
around 100-500 entries in the /etc/sub[ug]id. I think that this
implementation performs adequately in that area.
(I do hope I do NOT get famous for "500 entries in subuid ought to be
enough for everybody":)


b.

On 5 September 2014 23:53, Bostjan Skufca <bostjan at a2o.si> wrote:
>>> I created new branch for this one, for you to merge one single commig:
>>> https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2
>>
>> My 2c:
>> Do you need to include <grp.h>?
>
> Leftovers. When I started looking into this I was under impression
> that uidmap is determined by user's UID and gidmap is determined by
> user's GID. It turned out getgwnam() was not needed at all, as
> everything is UID-based.
>
>
>> You could avoid the getpwnam_r call (and malloc, sysconf) by calling
>> getpwnam. You just need to store owner_pwd.pw_uid before you enter the
>> loop.
>> This would also avoid the call to exit(), which IMO should be avoided in
>> commonio.c (better return an error code and let the applications deal with
>> the error - this can let them release a lock for example).
>
> So obvious :)
> (I copy-pasted the code strainght from getpwnam man page, that is why
> exit()s were there instead of proper error handling.)
>
> Unfortunately there is not a single instance of error handling in
> lib/subordinateio.c to take as example, and I am not sure that calling
> code knows how to handle errors other than "requested uidmap not
> allowed", which is what "return NULL;" stands for.
>
> Therefore ideas about how to do proper error handling are very
> welcome, but I think they belong to a separate thread.
>
>
>> I'm not sure regarding the guarantees you have on the size of pw_uid.
>> I would recommend:
>>         sprintf(owner_uid_string, "%lu", (unsigned long int)owner_pwd.pw_uid);
>
> Sensible, fixed.
>
>
>> This is rather cosmetic, but
>>                         if (!range_owner_pwd) {
>> could be changed to:
>>                         if (NULL == range_owner_pwd) {
>
> Future-proofing of course, fixed that too.
>
>
> Commit with fixes to review:
> https://github.com/bostjan/shadow/commit/d21214c9ba823edd5ac82ae2b1b03f4c219c3841
>
> Branch to actually merge (contains single commit with everything
> squashed into it):
> https://github.com/bostjan/shadow/tree/feature/subuidgid-numeric-v3
>
>
> @Serge:
> Maybe it would be for the best if you coordinated with Christian
> inclusion of this patch into upstream, as you are already "in the
> upstream-patch-pushing zone" :)
>
>
> b.