- Package:
- libnss-extrausers
- Source:
- libnss-extrausers
- Description:
- nss module to have an additional passwd, shadow and group file
- Submitter:
- Maik Zumstrull
- Date:
- 2024-07-24 16:21:03 UTC
- Severity:
- normal
The implementation uses three static global pointer variables: static FILE *groupsfile = NULL; static FILE *shadowfile = NULL; static FILE *usersfile = NULL; Since these are used without locks or atomic operations, this is not thread-safe, even though NSS functions are supposed to be reentrant. This is leading to occasional "double free or corruption" crashes in libvirtd for us, specifically in fclose(groupsfile); in _nss_extrausers_endgrent. As a quick fix, I suggest declaring these variables thread-local: static __thread FILE *groupsfile = NULL; static __thread FILE *shadowfile = NULL; static __thread FILE *usersfile = NULL; It's not necessarily the most elegant solution, but it should make the code fully reentrant.
Are you sure this wouldn't cause the different thread's understanding of
the underlying file descriptors to get out of sync? declaring them
thread-local wouldn't prevent them sharing the same file descriptor,
right? i think it would just be thread-local buffering.
This seems like a way to cause some even more subtle bug, for example:
thread A causes libnss-extrausers to scan the groups file for some
value, and it reads up to block X, while
thread B causes libnss-extrausers to scan the groups file for some
other value, and it reads up to block Y
then thread A resumes; the underlying file descriptor is advanced to
position Y but the FILE* buffers in thread A have only cached enough
data to represent block X. what happens then?
I could be misunderstanding the libnss-extrausers codebase, though --
feel free to clarify and correct!
It should. The semantics are that every thread "sees" its own version of that pointer, so if thread A opens the file, thread B, C, D, etc will still have NULL in their version of the variable and will do groupsfile = fopen(GROUPSFILE, "r"); which should create a new FILE structure and open a new file descriptor, whether any other thread has the file open or not. That's why I called the solution inelegant - it's not efficient to open the file seven times if seven threads want something - but it should be safe because nothing is shared between threads. That's the situation we have now, because everything from the FILE pointer to the FILE struct down to the file descriptor is currently shared.
This bug seems to have been inactive for many years. In the mean time there has been a discussion on https://sourceware.org/bugzilla/show_bug.cgi?id=27731 about getgrouplist(). The conclusion from this is that libnss-extrausers should supply a function _nss_extrausers_initgroups_dyn() that in a thread-safe way should provide the required information. Via https://forge.univention.org/bugzilla/show_bug.cgi?id=39775 it seems that somebody even made a patch which supplies that version, but then still uses the non-thread safe functions to implement it. Is there even still an upstream for this library? The only place I could find was with debian packages such as https://packages.debian.org/sid/libnss-extrausers . From there you can try to find an upstream in the .dsc file. That links to https://anonscm.debian.org/cgit/collab-maint/libnss-extrausers.git/log/?h=debian which no longer seems to exist... (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831390)