Repository navigation
Conversation
Remove access() prechecks before unlink() and fopen() and handle the result of the actual call instead: Vlib deletes support files unconditionally, G_get_projwkt and G_get_projsrid distinguish ENOENT from other fopen errors, d.mon deletes an existing output file based on unlink errno, and G__mkstemp only uses access() for the name-only (no-create) variant, relying on O_CREAT|O_EXCL otherwise. r.li now opens the configuration file first and sizes the buffer with fstat(). The dblogin file is now created with owner-only permissions atomically via open() instead of fopen() followed by chmod(). Fixes 11 CodeQL cpp/toctou-race-condition alerts.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
Bot
added
vector
CodeQL flagged the chmod(file, ...) as a time-of-check/time-of-use race: the file was already opened, but the permissions were then set by path, which could be redirected between the two operations. Set the permissions on the open descriptor with fchmod() instead. A newly created file already gets 0600 from open(O_CREAT, S_IRUSR | S_IWUSR) above; fchmod tightens a pre-existing file that open(O_TRUNC) leaves at its old mode, same as before but without the race. Fixes the CodeQL cpp/toctou-race-condition alert this PR reintroduced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses 11 of the 14 CodeQL
cpp/toctou-race-conditionalerts. These are check-then-use patterns on per-user mapset and config files, so none is a privilege boundary; the changes are correctness/robustness cleanups that also make the code simpler:Vlib/open.c,open_nat.c):access()beforeunlink()of topo/sidx/cidx/fidx support files.unlink()on a missing file is harmless, so the precheck is gone (a code comment inopen_nat.calready asked "remove this check?").get_projinfo.c):access()beforefopen()inG_get_projwkt()/G_get_projsrid(). Nowfopen()runs first andENOENTtakes the old "not found" path (including the PROJ_EPSG fallback); other errors stay fatal. Verified in both a projected project (WKT returned) and an XY project (returns NULL, no error).mkstemp.c): theaccess()existence probe is now only used for the name-onlyG_mktemp()variant (which is inherently just a probe, as its docs note); the creating variants rely solely on the existingO_CREAT|O_EXCL+EEXISThandling.start.c): delete an existing output file based onunlink()errno instead ofaccess()first; behavior (warning with--overwrite, fatal when undeletable) is unchanged.daemon.c): open the configuration file first, then size the read buffer withfstat()on the open descriptor instead ofstat()by path.login.c): the dblogin file is now created with owner-only (0600) permissions atomically viaopen(O_WRONLY|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR)+fdopen()instead offopen()followed bychmod(), so it never exists with wider permissions. Thechmod()remains to tighten a pre-existing file. Verified:db.loginproduces a-rw-------file with correct content. (MSVC keeps plainfopen(), matching the existing#ifndef _MSC_VERtreatment ofchmod.)The remaining 3 alerts (
lib/init/clean_temp.c) are left as is: thestat()result there decides file-vs-directory handling for the user's own tmp entries, the return values are already checked, and restructuring would not remove the race meaningfully. I propose dismissing those with justification.Verification: all five directories compile; runtime checks exercised each path (v.random + v.build topo rebuild,
g.proj -win projected and XY projects,d.mon startover an existing file with--overwrite,r.li.patchdensitywith a conf file,db.login).This is part of a larger effort to work through the open code scanning alerts, grouped into small PRs by issue type.
Written with the assistance of Claude Code.