On 15 Jul 2016, at 15:49, Daniel Kahn Gillmor <d...@fifthhorseman.net> wrote: > > On Fri 2016-07-15 14:19:51 +0200, Maik Zumstrull wrote:
>> 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? 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. > 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? 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.
