structcheck() builds its rename target with an unbounded sprintf#762
Merged
Conversation
Both structcheck() and structcheck_utf8() move a regular file sitting where a directory belongs, and build the "<name>.txt" target with a raw sprintf into a 2048-byte buffer. It stays in bounds only because of a strlen(path) > HTS_URLMAXSIZE guard dozens of lines above, which nothing at the write site mentions. Route both through sprintfbuff() and fail with ENAMETOOLONG, so the bound is local. Armed the probe: with that distant guard patched out, a 2045-byte path makes the old sprintf write 2049 bytes into tmpbuf[2048] under ASan; the same build with sprintfbuff() returns -1 and reports nothing. Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The path builder could end a path with a bare separator when the base directory's length hit the wrong residue, so the test aborted on a long $TMPDIR. The refusal case also asserted on a component structcheck never creates; assert on the outermost one instead. Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The path the guard admits (HTS_URLMAXSIZE) plus ".txt" is longer than macOS accepts, so fopen() failed there. The case could not tell a fixed build from an unfixed one anyway; what is left covers the guard and the rename on both entry points. Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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
1 participant
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.
structcheck()andstructcheck_utf8()move a regular file aside when a directory has to go where it sits, and both build the<name>.txtrename target with a rawsprintfinto a 2048-byte stack buffer. Nothing at the write says what keeps it in bounds. The answer is astrlen(path) > HTS_URLMAXSIZEcheck dozens of lines above, which reads as input validation rather than as the thing the buffer depends on. Both sites now go throughsprintfbuff()and return -1 with ENAMETOOLONG when the target does not fit.That failure branch is unreachable on today's tree: the guard caps the target at 1029 bytes of the 2048 available, so this is locality rather than a live bug fix. Patching the guard out and handing
structcheck()a 2045-byte path makes ASan report a 2049-byte write into the 2048-byte buffer; the same build withsprintfbuff()returns -1 and reports nothing.-#test=structcheckis therefore a characterization test, not a regression net for the overflow: it pins the guard and the rename for both the system-charset and the UTF-8 entry point. It deliberately does not try the rename at the guard's maximum length, since HTS_URLMAXSIZE plus ".txt" is longer than macOS will open.Closes #745