Skip to content

CodeQL guided medium risk (potential) file handle leaks path.c/flock.c #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dlmiles wants to merge 2 commits into from
Closed

CodeQL guided medium risk (potential) file handle leaks path.c/flock.c #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5c2b0e
path.c: CodeQL File{MayNot,Never}BeClosed.ql file-handle resource leaks
dlmiles Feb 13, 2025
1a139e7
CodeQL FileMayNotBeClosed.ql flock.c may also leak 'fd' in error paths
dlmiles Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions utils/flock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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;
}

Expand All @@ -156,7 +153,8 @@ gzFile flock_zopen(filename, mode, is_locked, fdp)
{
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re use of F_GETLK before F_SETLK

It seems strange to call F_GETLK first. The same process can not see its own lock, but it can see the lock of another process on the file(dev:ino). But if we are looking for another process with an active lock, there is a
race condition to this, because maybe it places down the lock between our call to F_GETLK and our F_SETLK.

So in effect any data obtained from doing things in this order is always out-of-date.

It is more usual to attempt to make the lock first, then if it fails, enquire with F_GETLK the PID of the process that is blocking us by using F_GETLK. So the nice error message can be displayed. Now if the F_GETLK fails to find a lock is set, maybe we just missed the other process unlocking, so we might (optionally) retry the operation of obtaining the lock one more time. Obviously we don't want to repeat more than once. But maybe magic files are not high-contention file (like database) so the retry maybe over the top.

perror(fname);
f = gzdopen(fd, mode);
goto done;
if (f)
goto done_store_fdp;
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vvvv Re Line 160 below vvvv

This mandatory call to close(fd) removes all locks our process has on this file. Another process can now gain their lock. Even if another part of our application has another FD open and will be doing more work on the FD later.
I don't get why a close() is made here, to a file that might be regarded as a lock aware file/path.

}
close(fd);
fd = -1;
Expand All @@ -172,14 +170,21 @@ gzFile flock_zopen(filename, mode, is_locked, fdp)
fd = open(fname, O_RDWR);
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^^ line 166 ^^^

Consider fl.l_type = ((oflags & O_ACCMODE) == O_RDONLY) ? F_RDLCK : F_WRLCK

Consider honouring the callers open mode flags for O_RDONLY and O_RDWR, I think maybe a O_RDONLY caller request is upgraded to O_RDWR with F_WRLCK. Unclear why that would be done.

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 */
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even when we don't get the lock, we carry on regardless, no indication or action made on error. A best-effort to obtain.

}
else
{
/* Diagnostic */
/* TxPrintf("Obtained lock on file <%s> (fd=%d)\n", fname, fd); */
}
f = gzdopen(fd, mode);
if (f == NULL)
{
close(fd);
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this close to manage the potential error path from the 3rd party library not accepting the 'fd'.
However this should also be a managed close if the file/path is consider file-lock aware.

fd = -1;
}
}
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are successful at this point here, we did fail to obtain the lock on this FD.
We did not mark *is_locked = TRUE informing the caller.
But... if the caller were to use close(fd) and there was another open FD on this same file(dev:ino) by another part of magic then it would release all our locks. Line 137-139 area and Line 160-161 area.
I think the caller will just use close(fd) like normal because *is_locked == TRUE. which removes all locks from the fd anyway.

So either this file is lock aware or it isn't. Given is_locked != NULL we can assume the caller is classifying this open call is being lock aware. If this is correct then all calls to open should have their fd managed

else
{
Expand All @@ -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;
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move because the function path_gzdopen_internal() will take care of this when successful and only one exit path exists no in this function that needs to ensure it is done here. So special goto label for that one use case'

freeMagic(fname);
return f;
}
Expand Down Expand Up @@ -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);
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rewrite of what is already there, in a form that is both similar to similar lines above and (I find) easier to see glance at its intention.

return f;
}

Expand Down Expand Up @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions utils/magic_zlib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
60 changes: 38 additions & 22 deletions utils/path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,38 @@ 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.
* Made non-static as flock() can use it but still considered module internal API.
*/
gzFile
path_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);
Copy link
Copy Markdown
Collaborator Author

@dlmiles dlmiles Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do anything for *fdp = -1 to handle error path. Maybe it should. None of previous code has it but looking back on it again I spot this.

if (fdp)
*fdp = -1;
return NULL;
}

/*
*-------------------------------------------------------------------
*
Expand Down Expand Up @@ -498,12 +530,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 path_gzdopen_internal(realName, oflag, mode, fdp);
}

/* If we were already given a full rooted file name,
Expand All @@ -522,12 +550,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 path_gzdopen_internal(realName, oflag, mode, fdp);
}

/* Now try going through the path, one entry at a time. */
Expand All @@ -540,13 +564,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 = path_gzdopen_internal(realName, oflag, mode, &fd);
#else
fd = open(realName, oflag);
f = gzdopen(fd, mode);
f = path_gzdopen_internal(realName, oflag, mode, &fd);
#endif

if (f != NULL)
Expand All @@ -571,13 +591,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 = path_gzdopen_internal(realName, oflag, mode, &fd);
#else
fd = open(realName, oflag);
f = gzdopen(fd, mode);
f = path_gzdopen_internal(realName, oflag, mode, &fd);
#endif

if (f != NULL)
Expand Down
Loading