#831390 static global variables are not thread safe

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
#831390#5
Date:
2016-07-15 12:19:51 UTC
From:
To:
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.

#831390#10
Date:
2016-07-15 13:49:36 UTC
From:
To:
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!

#831390#15
Date:
2016-07-15 14:01:50 UTC
From:
To:
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.

#831390#20
Date:
2024-07-24 16:12:35 UTC
From:
To:
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)