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

Bostjan Skufca bostjan at a2o.si
Fri Sep 5 21:53:40 UTC 2014

>> 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:

Branch to actually merge (contains single commit with everything
squashed into it):

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" :)


More information about the Pkg-shadow-devel mailing list