Skip to content

Commit 9abcafb

Browse files
klauspostklauspost
authored
s2: fix LZ4 block conversion writing past dst (#1244)
* s2: stop LZ4 block conversion from writing past dst The literal bound in the S2-output converters ignored the 1-5 byte literal header, leaving too little room for a following copy with a 5-byte repeat (up to 8 bytes). The assembly wrote a few bytes past cap(dst) before returning ErrDstTooSmall; the pure-Go path panicked. Matches over ~16.8 MB chained 5-byte repeat chunks with no check, so the overrun grew with the match length. Tighten the existing literal bound so a copy always fits, and check the limit only before each extra repeat chunk. The Snappy-output converters already checked every chunk. Add an unexported noAsm field so tests reach the pure-Go path without build tags. * s2: fuzz LZ4 conversion with under- and oversized dst FuzzLZ4Block gave the converter 2*len(data)+4096 bytes, which S2 output never comes close to, and asm writes past cap(dst) are invisible without a guard. Convert again with every variant into need +/- a deterministic amount up to len(data), with 16 guard bytes after cap, and check the guard and that the result agrees with the large-dst run. On the unfixed code this found an overrun within minutes; the input is added to the seed corpus. * s2: fuzz LZ4 assembly only when available FuzzLZ4Block covers the assembly and pure-Go converters in one run, so add the assembly variants only when hasAsm is set, and run the fuzzer in CI without the noasm,nounsafe pass, on both amd64 and arm64.
1 parent 4e2dcf8 commit 9abcafb

8 files changed

Lines changed: 1042 additions & 877 deletions

File tree

‎.github/workflows/go.yml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ jobs:
175175
test: [
176176
flate/FuzzEncoding, flate/FuzzInflateBufio,
177177
huff0/FuzzCompress, huff0/FuzzDecompress1x,
178-
s2/FuzzDictBlocks, s2/FuzzEncodingBlocks, s2/FuzzStreamDecode, s2/FuzzDecodeBlock, s2/FuzzLZ4Block,
178+
s2/FuzzDictBlocks, s2/FuzzEncodingBlocks, s2/FuzzStreamDecode, s2/FuzzDecodeBlock,
179179
snappy/xerial/FuzzDecode, snappy/xerial/FuzzEncode,
180180
zip/FuzzReader,
181181
zstd/FuzzDecodeAll, zstd/FuzzDecoder, zstd/FuzzEncoding,
@@ -184,6 +184,9 @@ jobs:
184184
# Skipped unless the amd64 BMI2 asm is available.
185185
- { os: ubuntu-latest, tags: '', test: zstd/FuzzDecAllNoBMI2 }
186186
- { os: ubuntu-latest, tags: '', test: zstd/FuzzNoBMI2Dec }
187+
# Covers the assembly and pure Go paths in one run.
188+
- { os: ubuntu-latest, tags: '', test: s2/FuzzLZ4Block }
189+
- { os: ubuntu-24.04-arm, tags: '', test: s2/FuzzLZ4Block }
187190
# Pure Go: neither tags nor arch change the code under test.
188191
- { os: ubuntu-latest, tags: '', test: fse/FuzzCompress }
189192
- { os: ubuntu-latest, tags: '', test: fse/FuzzDecompress }

‎s2/_generate/gen.go‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,7 @@ func (o options) genEncodeBlockAsm(name string, tableBits, skipLog, hashBytes, m
544544
JZ(LabelRef("repeat_as_copy_" + name))
545545

546546
// Emit as repeat...
547-
o.emitRepeat("match_repeat_"+name, length, offsetVal, nil, dst, LabelRef("repeat_end_emit_"+name), false)
547+
o.emitRepeat("match_repeat_"+name, length, offsetVal, nil, dst, nil, LabelRef("repeat_end_emit_"+name), false)
548548

549549
// Emit as copy instead...
550550
Label("repeat_as_copy_" + name)
@@ -1251,7 +1251,7 @@ func (o options) genEncodeBetterBlockAsm(name string, lTableBits, sTableBits, sk
12511251
MOVL(repeatL, offsetVal)
12521252

12531253
// Emit as repeat...
1254-
o.emitRepeat("match_repeat_"+name, length, offsetVal, nil, dst, LabelRef("repeat_end_emit_"+name), false)
1254+
o.emitRepeat("match_repeat_"+name, length, offsetVal, nil, dst, nil, LabelRef("repeat_end_emit_"+name), false)
12551255

12561256
Label("repeat_end_emit_" + name)
12571257
// Store new dst and nextEmit
@@ -1460,7 +1460,7 @@ func (o options) genEncodeBetterBlockAsm(name string, lTableBits, sTableBits, sk
14601460
// length += 4
14611461
ADDL(U8(4), length.As32())
14621462
MOVL(s, nextEmitL) // nextEmit = s
1463-
o.emitRepeat("match_nolit_repeat_"+name, length, offset, nil, dst, LabelRef("match_nolit_emitcopy_end_"+name), false)
1463+
o.emitRepeat("match_nolit_repeat_"+name, length, offset, nil, dst, nil, LabelRef("match_nolit_emitcopy_end_"+name), false)
14641464
}
14651465
}
14661466
Label("match_nolit_emitcopy_end_" + name)
@@ -1994,7 +1994,7 @@ func (o options) genEmitRepeat() {
19941994
Load(Param("dst").Base(), dstBase)
19951995
Load(Param("offset"), offset)
19961996
Load(Param("length"), length)
1997-
o.emitRepeat("standalone", length, offset, retval, dstBase, LabelRef("gen_emit_repeat_end"), false)
1997+
o.emitRepeat("standalone", length, offset, retval, dstBase, nil, LabelRef("gen_emit_repeat_end"), false)
19981998
Label("gen_emit_repeat_end")
19991999
Store(retval, ReturnIndex(0))
20002000
RET()
@@ -2006,8 +2006,10 @@ func (o options) genEmitRepeat() {
20062006
// retval can be nil.
20072007
// Will jump to end label when finished.
20082008
// Uses 1 GP register.
2009+
// dstLimit is optional. If set, each extra chunk of a repeat longer than
2010+
// 24 bits checks if dstLimit is reached and jumps to "end" without emitting the rest.
20092011
// longer indicates we know match will be > 12
2010-
func (o options) emitRepeat(name string, length reg.GPVirtual, offset reg.GPVirtual, retval reg.GPVirtual, dstBase reg.GPVirtual, end LabelRef, longer bool) {
2012+
func (o options) emitRepeat(name string, length reg.GPVirtual, offset reg.GPVirtual, retval reg.GPVirtual, dstBase, dstLimit reg.GPVirtual, end LabelRef, longer bool) {
20112013
Comment("emitRepeat")
20122014
if longer {
20132015
// Skip initial length tests
@@ -2057,6 +2059,10 @@ func (o options) emitRepeat(name string, length reg.GPVirtual, offset reg.GPVirt
20572059
if retval != nil {
20582060
ADDQ(U8(5), retval)
20592061
}
2062+
if dstLimit != nil {
2063+
CMPQ(dstBase, dstLimit)
2064+
JAE(end)
2065+
}
20602066
JMP(LabelRef("emit_repeat_again_" + name))
20612067
} else {
20622068
// Not needed.
@@ -2261,7 +2267,7 @@ func (o options) emitCopy(name string, length, offset, retval, dstBase, dstLimit
22612267
// return 5 + emitRepeat(dst[5:], offset, length)
22622268
// Inline call to emitRepeat. Will jump to end
22632269
if !o.snappy {
2264-
o.emitRepeat(name+"_emit_copy", length, offset, retval, dstBase, end, false)
2270+
o.emitRepeat(name+"_emit_copy", length, offset, retval, dstBase, dstLimit, end, false)
22652271
} else {
22662272
checkDst(dstBase)
22672273
JMP(LabelRef("four_bytes_loop_back_" + name))
@@ -2329,7 +2335,7 @@ func (o options) emitCopy(name string, length, offset, retval, dstBase, dstLimit
23292335
ADDQ(U8(2), dstBase) // dst += 2
23302336
SUBL(U8(8), length.As32()) // length -= 8
23312337
// emitRepeat(dst[2:], offset, length)
2332-
o.emitRepeat(name+"_emit_copy_short_2b", length, offset, retval, dstBase, end, true)
2338+
o.emitRepeat(name+"_emit_copy_short_2b", length, offset, retval, dstBase, dstLimit, end, true)
23332339

23342340
Label("long_offset_short_" + name)
23352341
}
@@ -2355,7 +2361,7 @@ func (o options) emitCopy(name string, length, offset, retval, dstBase, dstLimit
23552361
}
23562362
// Inline call to emitRepeat. Will jump to end
23572363
if !o.snappy {
2358-
o.emitRepeat(name+"_emit_copy_short", length, offset, retval, dstBase, end, false)
2364+
o.emitRepeat(name+"_emit_copy_short", length, offset, retval, dstBase, dstLimit, end, false)
23592365
} else {
23602366
checkDst(dstBase)
23612367
JMP(LabelRef("two_byte_offset_" + name))
@@ -3020,6 +3026,12 @@ func (o options) cvtLZ4BlockAsm(lz4s bool) {
30203026
srcEnd, dstEnd := GP64(), GP64()
30213027
LEAQ(Mem{Base: src, Index: srcLen, Scale: 1, Disp: 0}, srcEnd)
30223028
LEAQ(Mem{Base: dst, Index: dstLen, Scale: 1, Disp: -o.outputMargin}, dstEnd)
3029+
litEnd := dstEnd
3030+
if !o.snappy {
3031+
// Leave room for a 5-byte literal header and an 8-byte copy.
3032+
litEnd = GP64()
3033+
LEAQ(Mem{Base: dstEnd, Disp: -4}, litEnd)
3034+
}
30233035
lastOffset := GP64()
30243036
if !o.snappy {
30253037
XORQ(lastOffset, lastOffset)
@@ -3084,9 +3096,10 @@ func (o options) cvtLZ4BlockAsm(lz4s bool) {
30843096
TESTQ(ll, ll)
30853097
JZ(LabelRef(name + "lits_done"))
30863098
{
3087-
dstEnd := GP64()
3088-
LEAQ(Mem{Base: dst, Index: ll, Scale: 1}, dstEnd)
3089-
checkDst(dstEnd)
3099+
litDstEnd := GP64()
3100+
LEAQ(Mem{Base: dst, Index: ll, Scale: 1}, litDstEnd)
3101+
CMPQ(litDstEnd, litEnd)
3102+
JAE(LabelRef(name + "dstfull"))
30903103
o.outputMargin++
30913104
ADDQ(ll, retval)
30923105
o.emitLiteral(strings.TrimRight(name, "_"), ll, nil, dst, src, LabelRef(name+"lits_emit_done"), true)
@@ -3164,7 +3177,7 @@ func (o options) cvtLZ4BlockAsm(lz4s bool) {
31643177
// Offsets can only be 16 bits
31653178
{
31663179
// emitRepeat16(dst[d:], offset, ml)
3167-
o.emitRepeat("lz4_s2", ml, offset, nil, dst, LabelRef(name+"loop"), false)
3180+
o.emitRepeat("lz4_s2", ml, offset, nil, dst, dstEnd, LabelRef(name+"loop"), false)
31683181
}
31693182
}
31703183
Label(name + "docopy")

0 commit comments

Comments
 (0)