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

Bostjan Skufca bostjan at a2o.si
Fri Sep 5 22:40:39 UTC 2014


Heh, I just retested this with 1 000 000 entries, but this time
entries were specified with UID instead of username. Result: 0.310s!

It seems that getpwnam() returns very quickly if you pass it UID
(string) instread of actual username.

Anyway, I have added a note to subuid/subgid man pages, pushed to -v3
branch this commit too.

b.




On 6 September 2014 00:14, Bostjan Skufca <bostjan at a2o.si> wrote:
> 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.



More information about the Pkg-shadow-devel mailing list