From 4a086030264f5cf423ea76453ef721e2c8e2e093 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 2 Nov 2018 12:31:19 -0400 Subject: [PATCH] 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. --- src/stdio/fclose.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/stdio/fclose.c b/src/stdio/fclose.c index 889b96d2..d594532b 100644 --- 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; } -- 2.40.0