From 1653b982af1cf270d22082c76e6ded596e34d368 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Thu, 13 Feb 2025 08:23:07 +0000 Subject: [PATCH] CodeQL FileMayNotBeClosed.ql flock.c may also leak 'fd' in error paths This is due to the transfer of ownership of the 'fd' into libz/gzip but the error handling wasn't always being checked. --- utils/flock.c | 34 +++++++++++++++++++++------------- utils/magic_zlib.h | 1 + utils/path.c | 17 +++++++++-------- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/utils/flock.c b/utils/flock.c index e5e98f66..6e419eab 100644 --- a/utils/flock.c +++ b/utils/flock.c @@ -128,10 +128,8 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) else if (mode[0] == 'w') oflag = (mode[1] == '+') ? O_APPEND : O_WRONLY; - fd = open(fname, oflag); - if (fdp != NULL) *fdp = fd; - freeMagic(fname); - return gzdopen(fd, mode); + f = path_gzdopen_internal(fname, oflag, mode, fdp); + goto done; } /* Diagnostic */ @@ -141,8 +139,7 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) if (fd < 0) { if (is_locked) *is_locked = TRUE; - fd = open(fname, O_RDONLY); - f = gzdopen(fd, "r"); + f = path_gzdopen_internal(fname, O_RDONLY, "r", fdp); goto done; } @@ -156,7 +153,8 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) { perror(fname); f = gzdopen(fd, mode); - goto done; + if (f) + goto done_store_fdp; } close(fd); fd = -1; @@ -172,7 +170,9 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) fd = open(fname, O_RDWR); if (fcntl(fd, F_SETLK, &fl)) { - perror(fname); + perror(fname); + /* appears to be a best-effort rather than signal error, */ + /* and continues to gzdopen() to provide caller handle */ } else { @@ -180,6 +180,11 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) /* TxPrintf("Obtained lock on file <%s> (fd=%d)\n", fname, fd); */ } f = gzdopen(fd, mode); + if (f == NULL) + { + close(fd); + fd = -1; + } } else { @@ -191,12 +196,13 @@ gzFile flock_zopen(filename, mode, is_locked, fdp) TxPrintf("File <%s> is already locked by pid %d. Opening read-only.\n", fname, (int)fl.l_pid); if (is_locked) *is_locked = TRUE; - fd = open(fname, O_RDONLY); - f = gzdopen(fd, "r"); + f = path_gzdopen_internal(fname, O_RDONLY, "r", fdp); + goto done; } +done_store_fdp: + if (fdp) *fdp = fd; done: - if (fdp != NULL) *fdp = fd; freeMagic(fname); return f; } @@ -236,7 +242,8 @@ FILE *flock_open(filename, mode, is_locked, fdp) if (is_locked == NULL) { f = fopen(filename, mode); - if ((fdp != NULL) && (f != NULL)) *fdp = fileno(f); + if (f) + if (fdp) *fdp = fileno(f); return f; } @@ -298,7 +305,8 @@ FILE *flock_open(filename, mode, is_locked, fdp) } done: - if ((fdp != NULL) && (f != NULL)) *fdp = fileno(f); + if (f) + if (fdp) *fdp = fileno(f); return f; } diff --git a/utils/magic_zlib.h b/utils/magic_zlib.h index a052b99b..4398d8e9 100644 --- a/utils/magic_zlib.h +++ b/utils/magic_zlib.h @@ -29,6 +29,7 @@ extern gzFile PaZOpen(const char *file, const char *mode, const char *ext, const extern gzFile PaLockZOpen(const char *file, const char *mode, const char *ext, const char *path, const char *library, char **pRealName, bool *is_locked, int *fdp); extern char *PaCheckCompressed(const char *filename); +extern gzFile path_gzdopen_internal(const char *path, int oflags, const char *modestr, int *fdp); #ifdef FILE_LOCKS extern gzFile flock_zopen(); diff --git a/utils/path.c b/utils/path.c index 8f7c9422..a4ead248 100644 --- a/utils/path.c +++ b/utils/path.c @@ -68,9 +68,10 @@ bool FileLocking = TRUE; * but then static code analyser raises multiple concerns over multiple paths * leaking an fd. So at least this resolve these things and quietens the * output somewhat. + * Made non-static as flock() can use it but still considered module internal API. */ -static gzFile -gzdopen_internal(const char *path, int oflags, const char *modestr, int *fdp) +gzFile +path_gzdopen_internal(const char *path, int oflags, const char *modestr, int *fdp) { ASSERT(fdp, "fdp"); @@ -530,7 +531,7 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) return flock_zopen(realName, mode, is_locked, fdp); #endif - return gzdopen_internal(realName, oflag, mode, fdp); + return path_gzdopen_internal(realName, oflag, mode, fdp); } /* If we were already given a full rooted file name, @@ -550,7 +551,7 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) return flock_zopen(realName, mode, is_locked, fdp); #endif - return gzdopen_internal(realName, oflag, mode, fdp); + return path_gzdopen_internal(realName, oflag, mode, fdp); } /* Now try going through the path, one entry at a time. */ @@ -563,9 +564,9 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) f = flock_zopen(realName, mode, is_locked, &fd); else - f = gzdopen_internal(realName, oflag, mode, &fd); + f = path_gzdopen_internal(realName, oflag, mode, &fd); #else - f = gzdopen_internal(realName, oflag, mode, &fd); + f = path_gzdopen_internal(realName, oflag, mode, &fd); #endif if (f != NULL) @@ -590,9 +591,9 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) f = flock_zopen(realName, mode, is_locked, &fd); else - f = gzdopen_internal(realName, oflag, mode, &fd); + f = path_gzdopen_internal(realName, oflag, mode, &fd); #else - f = gzdopen_internal(realName, oflag, mode, &fd); + f = path_gzdopen_internal(realName, oflag, mode, &fd); #endif if (f != NULL)