From f8b78e9271600f67f7eafb09b66e00640f2c002f Mon Sep 17 00:00:00 2001 From: Vito Caputo Date: Fri, 12 Feb 2021 20:29:31 -0800 Subject: cache: _rmdCacheFileClose should always NULL handles The existing code would leave the internal handle non-NULL when the underlying close failed. In the event of a transparent close when chapters switch failing, a subsequent explicit close is expected, and the handles must be NULLed out for that explicit close to not blow up because fclose() leaves the handle undefined after errors. It's basically a use after free bug. --- src/rmd_cache.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/rmd_cache.c b/src/rmd_cache.c index 5b3323e..fc0b646 100644 --- a/src/rmd_cache.c +++ b/src/rmd_cache.c @@ -124,24 +124,34 @@ static int _rmdCacheFileOpen(CacheFile *file, const char *path) /* only close the internal file handle, but don't free file */ static int _rmdCacheFileClose(CacheFile *file) { + gzFile gzfp; + FILE *fp; + assert(file); + if (!(gzfp = file->gzfp) && !(fp = file->fp)) + return 0; + if (file->mode == RMD_CACHE_FILE_MODE_WRITE) { pthread_cancel(file->syncer_thread); pthread_join(file->syncer_thread, NULL); } + /* Always NULL out the handles, even if the close fails, otherwise + * a double-free kind of thing could occur. This is especially true + * for when a new chapter close fails (ENOSPC), an explicit close may + * still occur and if the handles are non-NULL things go boom. + */ + file->gzfp = NULL; + file->fp = NULL; + /* TODO: return meaningful -errno on errors? */ - if (file->gzfp) { - if (gzclose(file->gzfp) != Z_OK) + if (gzfp) { + if (gzclose(gzfp) != Z_OK) return -1; - - file->gzfp = NULL; - } else if (file->fp) { - if (fclose(file->fp)) + } else if (fp) { + if (fclose(fp)) return -1; - - file->fp = NULL; } return 0; -- cgit v1.2.3