unixpwd: fix reentrance of unixpwd_setpwd() and unixpwd_setspw()

[semarie]

• prefer fgetpwent_r() over getpwent_r() to avoid ETC_PASSWD FILE to be shared against threads
• prefer fgetspent_r() over getspent_r() to avoid ETC_SPASSWD FILE to be shared against threads
• while here, unlink(2) tmp_name on error in unixpwd_setpwd() to avoid leaving temporary file on error

[edwintorok]

There is a global lock around calls to these functions in the OCaml->C stubs (the OCaml runtime lock is never released).
Is the intention to call this function concurrently from different threads?

[edwintorok]

This one is still global, right?
What happens if 2 threads try to use lckpwdf? Do only one of them get it, do both of them get it?
Could it result in a bug where there are 2 callers to this function from 2 threads, the 1st one obtains the lock with lckpwdf(), the 2nd one (noop) acquires the lock again, and then the 1st one releases the lock with ulckpwdf(), leaving the 2nd thread to operate on the file without holding any locks, potentially corrupting it.

(If lckpwdf() relies on file-locks, then those only work per-process, and the application needs to do its own locking with mutexes, i.e. we're back to where we started, to having a global lock around this whole function)

[freddy77]

If 2 threads can obtain the lock at the same time it would be a very weird lock.
Yes, Glibc uses a file descriptor and a file lock so 2 threads in the same process will obtain the lock. How these functions are classified as multithread-safe I don't know...
BTW, the lock (if implemented, in MUSL they are empty stubs) should be global system wide, so it could block other processes.

[freddy77]

About Ocaml lock, yes, it should be released in unixpwd/c/unixpwd_stubs.c.

[edwintorok]

OK, but then we need to introduce another mutex here (which could still be beneficial, because it'd allow other OCaml threads to keep running, while this one deals with the password database, which is probably quite rare)

[edwintorok]

Although if the intention is just to get this to compile with Musl (#6729 (comment)), then keeping the OCaml runtime lock as is might be simpler (we don't gain any additional concurrency, and we don't risk introducing any concurrency bugs in our existing code either).

[semarie]

@edwintorok about the intent, in #6728 I proposed to replace getpwent_r() by getpwent() for enhance compatibility with alpine (musl libc). But concerns were raised about the thread safety. As getpwent_r() isn't thread safe neither, the purpose was to replace it with proper thread safe function (fgetpwent_r()).

But if OCaml runtime lock isn't released it might be overkill.

Regarding lckpwdf(), yes it is a global lock. But the documentation doesn't provide details:

The lckpwdf() function is intended to protect against multiple
simultaneous accesses of the shadow password database. It tries to
acquire a lock, and returns 0 on success, or -1 on failure (lock not
obtained within 15 seconds).

From glibc implementation, it should work against multiple threads in same process and against multiple processes (if all are using lckpwdf()).

[lindig]

This code is rarely used such that problems around concurrent access were unlikely to be noticed. I am happy to accept the improvements.

[edwintorok]

This code is rarely used such that problems around concurrent access were unlikely to be noticed. I am happy to accept the improvements.

I think the OCaml runtime lock would prevent concurrent accesses

@edwintorok about the intent, in #6728 I proposed to replace getpwent_r() by getpwent() for enhance compatibility with alpine (musl libc).

Although if some libcs don't provide the thread-unsafe variants, then that is a good enough motive to switch to the more thread-safe ones, at least to keep things building, even if we don't release the OCaml runtime lock.

[semarie]

I am switching this in PR in draft for now, as there is no rush to push it:

• thread safety isn't a problem (due to OCaml runtime lock)
• as it, it doesn't help compat with musl as fgetpwent_r isn't present in musl neither

The purpose of this PR was specifically to address a (non-existent) thread safety issue, as my initial getpwent() proposal was withdrawn due to thread safety concerns.