Skip to content

Fix out-of-bounds slice in TrimLeadingSQLComments for single-character lines - #834

Merged
dossy merged 2 commits into
amacneil:mainfrom
MattBridges:fix/trim-leading-sql-comments-oob
Sep 15, 2026
Merged

dossy merged 2 commits into
amacneil:mainfrom
MattBridges:fix/trim-leading-sql-comments-oob

Conversation

@MattBridges

Copy link
Copy Markdown
Contributor

What

TrimLeadingSQLComments tests whether a preamble line is a comment via
bytes.Equal(line[0:2], []byte("--")), without first checking that the
line 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-character
data line gets misread as a comment and silently dropped from the
output — e.g. dropped from a schema.sql dump.

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 existing
zero-length-line handling right next to it.

Testing

Added TestTrimLeadingSQLCommentsSingleCharacterPreambleLine alongside
the existing TestTrimLeadingSQLComments. Verified via go test ./pkg/dbutil/... and go vet ./pkg/dbutil/... (Go 1.25, in Docker,
since this environment doesn't have a native Go toolchain) — full
dbutil package 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.

…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.
@dossy

dossy commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

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. bytes.HasPrefix instead of the length guard

The guard works, but bytes.HasPrefix expresses the intent directly and doesn't need one:

if preamble && (len(line) == 0 || bytes.HasPrefix(line, []byte("--"))) {

bytes is already imported, and this matches the guarded-index style already used at dbutil.go:25.

2. A test that actually fails without the fix

This is the substantive one. I reverted the one-line change and ran the added test against the unfixed code:

=== RUN   TestTrimLeadingSQLCommentsSingleCharacterPreambleLine
--- PASS

It can't fail. For a one-byte line in the middle of the input, the byte sitting immediately after the token in the scanner's buffer is always the \n that terminated it, so line[0:2] evaluates to "-\n" — never "--". The test documents the intended behavior but doesn't pin the bug, so it wouldn't catch a regression later.

Reproducing the bug requires the scanner to actually slide its buffer, which takes >4KB of preamble plus a final one-byte line with no trailing newline. This version fails before the change and passes after — I verified both directions:

func TestTrimLeadingSQLCommentsShortLineAtEOF(t *testing.T) {
	// >4KB of leading comments forces bufio.Scanner to slide its buffer,
	// leaving stale bytes just past the final token; a one-byte last line
	// must not be misread as a comment marker.
	var b strings.Builder
	for b.Len() < 4090 {
		b.WriteString("--\n")
	}
	for b.Len() < 4095 {
		b.WriteString("-")
	}
	b.WriteString("\n-") // final line: one byte, no trailing newline

	out, err := dbutil.TrimLeadingSQLComments([]byte(b.String()))
	require.NoError(t, err)
	require.Equal(t, "-\n", string(out))
}

3. Retitling, because the panic isn't reachable

cap(line) is bounded by the scanner's 4096-byte buffer, and bufio.Scanner slides start back to 0 before the at-EOF split, so the capacity behind the token is effectively always >= 4096. I couldn't construct a panic and I don't believe one exists. It's an out-of-length read inside a valid buffer, not a memory-safety issue.

The misclassification is the real bug, and it's worth fixing on correctness grounds alone, but it's narrow: it needs a multi-kilobyte all-comment preamble, a final line of exactly one byte, no trailing newline, and the stale byte left behind to happen to be -. Nothing pg_dump, mysqldump or sqlite3 emits looks like that, and the line that gets dropped is a bare -, which isn't valid SQL either. So I'd rather this land in the history as a latent correctness fix than as something that was eating real schema.sql output.

Since this repo squash-merges, the PR title and description are what end up in git log permanently, so I'm updating them to:

fix: don't drop one-character lines when trimming leading SQL comments

TrimLeadingSQLComments tested whether a preamble line was a comment using
bytes.Equal(line[0:2], []byte("--")) without first checking the line's
length. For a one-byte line this reads a byte past the token returned by
the scanner. The byte is still inside the scanner's buffer, so this does
not panic, but it is not part of the line.

Mid-input that trailing byte is always the newline that terminated the
line, so the comparison fails harmlessly. Once bufio.Scanner slides its
buffer, though, a one-byte final line with no trailing newline can be
followed by a stale '-' left over from earlier content. The line is then
misread as a comment and silently dropped from the output.

Use bytes.HasPrefix, which handles short lines by construction, and add a
regression test that reproduces the dropped line.

The fix is yours and the authorship stays yours — I'm only adjusting how it's described and tightening the test. Shout if you disagree with any of it.

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.
@dossy
dossy merged commit 24c68be into amacneil:main Sep 15, 2026
10 checks passed
@dossy dossy mentioned this pull request Sep 19, 2026
dossy added a commit that referenced this pull request Sep 19, 2026
Changes since v2.35.1:

* #835
* #803
* #834
* #827
* #785
* #833
* #825
* #823
* #837
* #705
* #814
* #840
* #826
* #841
* #839, #831
* #836
* #832
* #830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TrimLeadingSQLComments reads past line bounds on single-character lines (potential panic / wrong skip)

2 participants