TrimLeadingSQLComments reads past line bounds on single-character lines (potential panic / wrong skip)
- 主要語言
- Go
- 星號
- 7.4k
- 分支
- 380
- 平均合併
- 1 天 16 小時
- 30 天內合併 PR
- 2
描述
## Description
`dbutil.TrimLeadingSQLComments` indexes `line[0:2]` without checking that the line has at least two bytes. For a one-character line inside the leading-comment block, this either panics or silently compares bytes beyond the end of the current line, so the function can crash or wrongly drop a data line from a schema dump.
*This report is based on static analysis of the source (master); I have not executed the code.*
- Version: master (`pkg/dbutil/dbutil.go`)
- Database: affects all drivers using this helper for dump post-processing (postgres, mysql, sqlite)
- Operating System: any
## Location
- File: `pkg/dbutil/dbutil.go`
- Function: `TrimLeadingSQLComments`
- Relevant code path:
```go
scanner := bufio.NewScanner(bytes.NewReader(data))
for scanner.Scan() {
line := scanner.Bytes()
if preamble && (len(line) == 0 || bytes.Equal(line[0:2], []byte("--"))) {
continue
}
...
```
## Problem
The preamble check tests `len(line) == 0 || bytes.Equal(line[0:2], []byte("--"))`. When `len(line) == 1`, the expression `line[0:2]` slices past the end of the token:
1. **Panic case:** Go slicing requires `high <= cap(s)`. If the underlying array shared by `scanner.Bytes()` does not have two addressable bytes after the token start, `line[0:2]` panics with *slice bounds out of range*. `bufio.Scanner.Bytes()` returns a view into the scanner's internal buffer, whose remaining capacity at that offset depends on buffer growth/sliding — so whether this panics depends on where the line sits in the buffer, not just on its length.
2. **Wrong-comparison case:** when capacity happens to be sufficient, `bytes.Equal(line[0:2], ...)` still reads a byte that is not part of the current line (the following newline, or stale bytes from a previous read chunk after the scanner reuses its buffer). The comparison result is then determined by unrelated buffer content: if those bytes happen to be `"--"`, a legitimate one-character data line is silently skipped and removed from the dump; otherwise it is kept.
## Steps To Reproduce
Conceptually, call:
```go
dbutil.TrimLeadingSQLComments([]byte("-- header\n-\nSELECT 1;\n"))
```
The second line (`-`, length 1) reaches the `preamble` branch with `len(line) == 1`, forcing evaluation of `line[0:2]`. Whether this panics or miscompares depends on the scanner's internal buffer state at that offset (see above). A unit test with inputs engineered around the ~4KB scanner-buffer boundary should expose both modes.
## Expected Behavior
A one-character line should simply fail the "is it a comment?" test (it cannot start with `--`) and be preserved in the output, without any out-of-bounds access.
## Actual Behavior
`line[0:2]` performs an out-of-bounds-capacity slice for length-1 lines: potential panic, and an unreliable comparison that can drop lines from schema dumps processed by `DumpSchema` implementations.
## Impact
`TrimLeadingSQLComments` post-processes `pg_dump`/`mysqldump` output in the postgres, mysql and sqlite drivers. A crash aborts dumping; the wrong-skip mode would corrupt `schema.sql` by deleting a real line from the dumped SQL. Both failure modes are silent or confusing for users.
## Suggested Direction
Guard the slice, e.g. `len(line) >= 2 && bytes.Equal(line[0:2], []byte("--"))` (the zero-length case is already handled), and add table-driven cases with single-character lines inside and after the preamble.
貢獻指南
這個儲存庫沒有索引到貢獻指南
研究方向
Start in pkg/dbutil/dbutil.go at TrimLeadingSQLComments and inspect how bufio.Scanner processes each line. Add table-driven coverage for single-character lines inside and after the leading-comment block, including the provided dump-shaped input, then run the relevant Go package tests and confirm those lines are preserved without a panic.
由索引模型根據 Issue 內容生成。
評估
- 技術堆疊
- go, mysql, postgresql, sqlite
- 領域
- databases
- Issue 類型
- 缺陷
- 難度
- 2/5
- 預估耗時
- 1-3 小時
- 活躍度
- 活躍
- 描述清晰度
- 描述清楚
- 新手友好度
- 88/100