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

@@ -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);
+	f = path_gzdopen_internal(fname, oflag, mode, fdp);
-	if (fdp != NULL) *fdp = fd;
+	goto done;
-	freeMagic(fname);
-	return gzdopen(fd, mode);
     }
 
     /* 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 = path_gzdopen_internal(fname, O_RDONLY, "r", fdp);
-	f = gzdopen(fd, "r");
 	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;
@@ -173,6 +171,8 @@ gzFile flock_zopen(filename, mode, is_locked, fdp)
 	if (fcntl(fd, F_SETLK, &fl))
 	{
 	    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 = path_gzdopen_internal(fname, O_RDONLY, "r", fdp);
-	f = gzdopen(fd, "r");
+	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;
 }
 

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();

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
+gzFile
-gzdopen_internal(const char *path, int oflags, const char *modestr, int *fdp)
+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)