From d5c2b0ef9df79da25d17f74050dc8f26258fd993 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Thu, 13 Feb 2025 08:22:28 +0000 Subject: [PATCH 1/2] path.c: CodeQL File{MayNot,Never}BeClosed.ql file-handle resource leaks Guided by CodeQL static code analyser. FileMayNotBeClosed.ql FileMayNeverBeClosed.ql Multiple paths exist that seems to be equivalent, some of those paths handled the error path, some of them didn't. CodeQL will still report a concern (instead of multiple concerns) so this has been commented to clarify when the libz handle setup is successful, the ownership of the 'fd' changed. --- utils/path.c | 59 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/utils/path.c b/utils/path.c index decc84f38..8f7c94228 100644 --- a/utils/path.c +++ b/utils/path.c @@ -63,6 +63,37 @@ bool FileLocking = TRUE; #define MAXSIZE MAXPATHLEN #ifdef HAVE_ZLIB +/* this was found repeated a number of times, only sometimes checking NULL + * return from gzdopen() which is unlikely/difficult to ever see from libz, + * 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. + */ +static gzFile +gzdopen_internal(const char *path, int oflags, const char *modestr, int *fdp) +{ + ASSERT(fdp, "fdp"); + + int fd = open(path, oflags); + if (fd < 0) + return NULL; + + /* when gzdopen() successful ownership of fd is transferred */ + gzFile gzhandle = gzdopen(fd, modestr); + if (gzhandle == NULL) + goto close; + + if (fdp) /* but sometimes the caller still wants to know the fd */ + *fdp = fd; + return gzhandle; + +close: + close(fd); + if (fdp) + *fdp = -1; + return NULL; +} + /* *------------------------------------------------------------------- * @@ -498,12 +529,8 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) #ifdef FILE_LOCKS if (FileLocking) return flock_zopen(realName, mode, is_locked, fdp); - else #endif - fd = open(realName, oflag); - - if (fdp != NULL) *fdp = fd; - return gzdopen(fd, mode); + return gzdopen_internal(realName, oflag, mode, fdp); } /* If we were already given a full rooted file name, @@ -522,12 +549,8 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) #ifdef FILE_LOCKS if (FileLocking) return flock_zopen(realName, mode, is_locked, fdp); - else #endif - fd = open(realName, oflag); - - if (fdp != NULL) *fdp = fd; - return gzdopen(fd, mode); + return gzdopen_internal(realName, oflag, mode, fdp); } /* Now try going through the path, one entry at a time. */ @@ -540,13 +563,9 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) f = flock_zopen(realName, mode, is_locked, &fd); else - { - fd = open(realName, oflag); - f = gzdopen(fd, mode); - } + f = gzdopen_internal(realName, oflag, mode, &fd); #else - fd = open(realName, oflag); - f = gzdopen(fd, mode); + f = gzdopen_internal(realName, oflag, mode, &fd); #endif if (f != NULL) @@ -571,13 +590,9 @@ PaLockZOpen(file, mode, ext, path, library, pRealName, is_locked, fdp) if (FileLocking) f = flock_zopen(realName, mode, is_locked, &fd); else - { - fd = open(realName, oflag); - f = gzdopen(fd, mode); - } + f = gzdopen_internal(realName, oflag, mode, &fd); #else - fd = open(realName, oflag); - f = gzdopen(fd, mode); + f = gzdopen_internal(realName, oflag, mode, &fd); #endif if (f != NULL) From 1a139e77c7483a32069df2575ff69c960b62bce6 Mon Sep 17 00:00:00 2001 From: "Darryl L. Miles" Date: Thu, 13 Feb 2025 08:23:07 +0000 Subject: [PATCH 2/2] 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 e5e98f661..6e419eabb 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 a052b99b8..4398d8e92 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 8f7c94228..a4ead2486 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)