Fix out-of-bounds slice in TrimLeadingSQLComments for single-character lines - #834
Conversation
…r lines len(line) was not checked before slicing line[0:2] when testing whether a preamble line starts with "--". A one-character line inside the leading-comment block can slice past its own bounds (within the scanner buffer's capacity, so this doesn't reliably panic, but reads a byte that isn't part of the line and can misclassify it as a comment, silently dropping a real line from a schema dump). Guard the length before slicing, and add a regression test.
|
Thanks for the patch — the underlying defect is real and the fix is correct. I checked this out locally and verified the behavior in both directions. Rather than send you round another review cycle for something this small, I'm going to push three changes to the branch; here's the reasoning for each. 1.
|
Replace the length-guarded index with bytes.HasPrefix, which handles short lines by construction and matches the intent of the check. The previous regression test could not fail. Mid-input, the byte after a one-byte token is always the newline that terminated it, so line[0:2] evaluates to "-\n" and never matches "--". Reproducing the bug requires bufio.Scanner to slide its buffer, which takes more than 4KB of preamble plus a one-byte final line with no trailing newline, leaving a stale '-' just past the token. The replacement test was verified failing before the fix and passing after.
What
TrimLeadingSQLCommentstests whether a preamble line is a comment viabytes.Equal(line[0:2], []byte("--")), without first checking that theline has at least 2 bytes. For a one-character line inside the leading
comment block, this slices past the line's own length (bounded only by
the scanner buffer's remaining capacity at that offset, so it usually
doesn't panic, but it does read a byte that isn't part of the current
line). If that stray byte happens to be
-, a legitimate one-characterdata line gets misread as a comment and silently dropped from the
output — e.g. dropped from a
schema.sqldump.Fix
Guard the length before slicing:
len(line) >= 2 && bytes.Equal(line[0:2], ...).A one-character line now simply fails the "is this a comment?" check (it
can't start with
--) and is preserved, matching the existingzero-length-line handling right next to it.
Testing
Added
TestTrimLeadingSQLCommentsSingleCharacterPreambleLinealongsidethe existing
TestTrimLeadingSQLComments. Verified viago test ./pkg/dbutil/...andgo vet ./pkg/dbutil/...(Go 1.25, in Docker,since this environment doesn't have a native Go toolchain) — full
dbutilpackage test suite passes, including the new case.Context
I ran into this while reading through dbmate's dump post-processing
code — I maintain a small free CLI for a different but related problem
(catching unsafe schema migrations before they run, for Cloudflare D1:
https://github.com/MattBridges/d1-migration-guard), and was looking at
how other migration tools handle SQL text processing. Wanted to send
the fix rather than just an issue report since it's small and I'd
already verified it. Happy to adjust the test or fix if you'd prefer a
different approach.