fix deadlock and buffered data loss race in fclose

fflush(NULL) and __stdio_exit lock individual FILEs while holding the
open file list lock to walk the list. since fclose first locked the
FILE to be closed, then the ofl lock, it could deadlock with these
functions.

also, because fclose removed the FILE to be closed from the open file
list before flushing and closing it, a concurrent fclose or exit could
complete successfully before fclose flushed the FILE it was closing,
resulting in data loss.

reorder the body of fclose to first flush and close the file, then
remove it from the open file list only after unlocking it. this
creates a window where consumers of the open file list can see dead
FILE objects, but in the absence of undefined behavior on the part of
the application, such objects will be in an inactive-buffer state and
processing them will have no side effects.

__unlist_locked_file is also moved so that it's performed only for
non-permanent files. this change is not necessary, but preserves
consistency (and thereby provides safety/hardening) in the case where
an application uses one of the standard streams after closing it while
holding an explicit lock on it. such usage is of course undefined
behavior.

diff --git a/src/stdio/fclose.c b/src/stdio/fclose.c
index 889b96d..d594532 100644 (file)
--- a/src/stdio/fclose.c
+++ b/src/stdio/fclose.c
@@ -7,26 +7,32 @@ weak_alias(dummy, __unlist_locked_file);
 int fclose(FILE *f)
 {
        int r;
-       int perm;
        
        FLOCK(f);
+       r = fflush(f);
+       r |= f->close(f);
+       FUNLOCK(f);
 
-       __unlist_locked_file(f);
+       /* Past this point, f is closed and any further explict access
+        * to it is undefined. However, it still exists as an entry in
+        * the open file list and possibly in the thread's locked files
+        * list, if it was closed while explicitly locked. Functions
+        * which process these lists must tolerate dead FILE objects
+        * (which necessarily have inactive buffer pointers) without
+        * producing any side effects. */
 
-       if (!(perm = f->flags & F_PERM)) {
-               FILE **head = __ofl_lock();
-               if (f->prev) f->prev->next = f->next;
-               if (f->next) f->next->prev = f->prev;
-               if (*head == f) *head = f->next;
-               __ofl_unlock();
-       }
+       if (f->flags & F_PERM) return r;
 
-       r = fflush(f);
-       r |= f->close(f);
+       __unlist_locked_file(f);
+
+       FILE **head = __ofl_lock();
+       if (f->prev) f->prev->next = f->next;
+       if (f->next) f->next->prev = f->prev;
+       if (*head == f) *head = f->next;
+       __ofl_unlock();
 
        free(f->getln_buf);
-       if (!perm) free(f);
-       else FUNLOCK(f);
+       free(f);
 
        return r;
 }