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

Sat Sep 6 18:51:17 UTC 2014

Very reasonable, fixed, pushed.

b.

On 6 September 2014 10:10, Nicolas François
<nicolas.francois at centraliens.net> wrote:
> Hi,
>
> The performance hit with getpw* is getting much worse when you have a remote
> database (e.g ldap).
>
> You may consider checking the range first as this does not require syscall,
> and then checking the user.
>
> Kind regards,
> --
> Nekral
>
>
> Le samedi 6 septembre 2014, <bostjan at a2o.si> a écrit :
>
>> 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.
>>
>
> --
> Nekral