Use getpwnam_r instead of getpwnam in multi-threaded code

[pabloyoyoista]

This pull request is an attempt to provide an upstream fix to multiple crashes seen in Alpine with cups. The crashes have in common that they happen on calls to getpwnam. Reading the documentation, I found that getpwnam and getpwuid are thread-unsafe functions. If I understand it correctly, the code under the cups folder is (potentially?) multi-threaded and could explain the crashes in getpwnam. Substituting all calls by getpwnam_r and getpwuid_r fix the crashes, at least in my limited testing.

Two things to note:

• getpwnam and getpwuid are marked thread-unsafe by musl/POSIX, but also by glibc. Therefore, although I have not seen reports of crashes happening with glibc, they could potentially happen in the future if this fix is not applied in multi-threaded code.
• There are also many calls to getpwnam and getpwuid under different files in the scheduler folder. However, if I understand it correctly by the documentation, the scheduler is single-threaded and using getpwnam and getpwuid should be safe. Otherwise, should I also change the calls there?

[michaelrsweet]

Thanks for your contribution!

I'm 100% in favor of switching to the reentrant versions of getpwnam/uid, however the current code doesn't follow the coding style documented in the DEVELOPING.md file, and we need to add conditional code for the sysconf call (a simple #ifdef on the _SC_GETPW_R_SIZE_MAX macro should do) or you can just put a large char array in the _cups_globals_t structure to have a per-thread buffer to avoid allocating on the fly.

[zdohnal]

Thank you for the PR!

Ideally it would be great if the PR had config script checks for new functions and macros (getpwnam_r, getpwuid_r, sysconf, _SC_GETPW_R_SIZE_MAX) and have fallbacks if some of them aren't available on target system.
I can imagine there are systems where some of those aren't available...

[pabloyoyoista]

the current code doesn't follow the coding style documented in the DEVELOPING.md file,

Sorry for that. I believe it should be following all the details now.

we need to add conditional code for the sysconf call (a simple #ifdef on the _SC_GETPW_R_SIZE_MAX macro should do) or you can just put a large char array in the _cups_globals_t structure to have a per-thread buffer to avoid allocating on the fly.

I have gone for using a large array in _cups_globals_t. Thank you for this suggestion, it greatly simplifies the code. However, there is still the question of the "magic number" mentioned by @zdohnal. I have used 16KB, as it is the fallback number recommended by glibc manual, but I cannot say much more about it. Do you have any comments/recommendations?

Ideally it would be great if the PR had config script checks for new functions and macros

Right now using, _cups_globals_t, the problem is limited to getpwnam_r and getpwuid_r. Making a check which checks for those functions and unconditionally fails if missing makes sense to me. However, if the aim is to provide a fallback, I really wonder how can we identify the right one.

I would assume that any unix-based system using getpwnam and getpwuid at this point follows POSIX 1003.1-2008, which already includes getpwnam_r and getpwuid_r, and states "The getpwnam() function need not be thread-safe." If my assumption is wrong, then is it safe to assume that such systems implement getpwnam as thread-safe? If they do, how do we identify it?

BTW, thank you very much for the feedback and sorry for my quite not-portable PR and ignorance in some of these points. I am coming from a strict Linux background and I really do not know many details for other systems.

[michaelrsweet]

@pablofsf getpwnam_r goes back to the 1996 version of POSIX, so unless somebody reports a platform that doesn't support it let's avoid the complexity of checking and working around a missing implementation of it. Some platforms (macOS is one) use per-thread storage for the non-reentrant versions already so they are thread-safe, but I don't see a reason to special-case that.

I think 16k is more than enough - the passwd structure only has 6 string members and most are typically blank or short strings. The only potentially long string is pw_dir(home directory) but that will be limited to PATHMAX (1024 on most systems).

@zdohnal At this point I think my only concern would be in using the sysconf constants without an #ifdef check, but with the buffer in the per-thread globals I'm good.

[pabloyoyoista]

Thank you so much for all the information! Should I send a similar PR for the calls under scheduler? Or is it ok to use thread-unsafe getpwnam there?

[michaelrsweet]

@pablofsf The current implementation of the scheduler is single-threaded (well, for the parts that use getpwnam anyways) so let's not worry about that.

[zdohnal]

@michaelrsweet ok, LGTM as well.

@pablofsf Thank you for the changes!

[zdohnal]

I've checked the code style and semantics of the code, looks great, merging.

[pabloyoyoista]

Thank you both! Looking forward to have this in the coming 2.4 release :))