From 0dac3fd19c432d5f53717a548832944aa453cdbd Mon Sep 17 00:00:00 2001
From: "Darryl L. Miles" <darryl.miles@darrylmiles.org>
Date: Thu, 13 Feb 2025 08:22:28 +0000
Subject: [PATCH] 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 decc84f3..8f7c9422 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)