internal/zstd: fix seek offset bounds check in skipFrame

This change enhances the zstd Reader's skipFrame function to validate
the new offset when skipping frames in a seekable stream, preventing
invalid offsets that could occur previously.

A set of "bad" test strings has been added to fuzz_test.go to extend
the robustness checks against potential decompression panics.

Additionally, a new test named TestReaderBad is introduced in
zstd_test.go to verify proper error handling with corrupted input
strings.

The BenchmarkLarge function has also been refactored for clarity,
removing unnecessary timer stops and resets.

Updates #63824

Change-Id: Iccd248756ad6348afa1395c7799350d07402868a
GitHub-Last-Rev: 63055b91e9413491fe8039ea42d55b823c89ec15
GitHub-Pull-Request: golang/go#64056
Reviewed-on: https://go-review.googlesource.com/c/go/+/541220
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Klaus Post <klauspost@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/internal/zstd/fuzz_test.go b/src/internal/zstd/fuzz_test.go
index 4c0e9cf7b9c7cb33effd01ef7b02a1aea68c2797..4b5c9961d87aee977ac654a6916d9c67402d8b7c 100644 (file)
--- a/src/internal/zstd/fuzz_test.go
+++ b/src/internal/zstd/fuzz_test.go
@@ -24,6 +24,7 @@ var badStrings = []string{
        "(\xb5/\xfd001\x00\x0000000000000000000",
        "(\xb5/\xfd00\xec\x00\x00&@\x05\x05A7002\x02\x00\x02\x00\x02\x0000000000000000",
        "(\xb5/\xfd00\xec\x00\x00V@\x05\x0517002\x02\x00\x02\x00\x02\x0000000000000000",
+       "\x50\x2a\x4d\x18\x02\x00\x00\x00",
 }
 
 // This is a simple fuzzer to see if the decompressor panics.

diff --git a/src/internal/zstd/zstd.go b/src/internal/zstd/zstd.go
index 9cf62a6bacad2d43129ffb09209a81b51a97f3f1..0230076f5074c5c46ae452e609655164165db142 100644 (file)
--- a/src/internal/zstd/zstd.go
+++ b/src/internal/zstd/zstd.go
@@ -326,12 +326,34 @@ func (r *Reader) skipFrame() error {
        relativeOffset += 4
 
        size := binary.LittleEndian.Uint32(r.scratch[:4])
+       if size == 0 {
+               r.blockOffset += int64(relativeOffset)
+               return nil
+       }
 
        if seeker, ok := r.r.(io.Seeker); ok {
-               if _, err := seeker.Seek(int64(size), io.SeekCurrent); err != nil {
-                       return err
+               r.blockOffset += int64(relativeOffset)
+               // Implementations of Seeker do not always detect invalid offsets,
+               // so check that the new offset is valid by comparing to the end.
+               prev, err := seeker.Seek(0, io.SeekCurrent)
+               if err != nil {
+                       return r.wrapError(0, err)
+               }
+               end, err := seeker.Seek(0, io.SeekEnd)
+               if err != nil {
+                       return r.wrapError(0, err)
+               }
+               if prev > end-int64(size) {
+                       r.blockOffset += end - prev
+                       return r.makeEOFError(0)
+               }
+
+               // The new offset is valid, so seek to it.
+               _, err = seeker.Seek(prev+int64(size), io.SeekStart)
+               if err != nil {
+                       return r.wrapError(0, err)
                }
-               r.blockOffset += int64(relativeOffset) + int64(size)
+               r.blockOffset += int64(size)
                return nil
        }
 

diff --git a/src/internal/zstd/zstd_test.go b/src/internal/zstd/zstd_test.go
index 4ae6f2b3982c1cc3ed88d393255b9b5b1c762393..f2a2e1b5851314a2c9ada47186320cc21e7c235c 100644 (file)
--- a/src/internal/zstd/zstd_test.go
+++ b/src/internal/zstd/zstd_test.go
@@ -304,6 +304,17 @@ func TestFileSamples(t *testing.T) {
        }
 }
 
+func TestReaderBad(t *testing.T) {
+       for i, s := range badStrings {
+               t.Run(fmt.Sprintf("badStrings#%d", i), func(t *testing.T) {
+                       _, err := io.Copy(io.Discard, NewReader(strings.NewReader(s)))
+                       if err == nil {
+                               t.Error("expected error")
+                       }
+               })
+       }
+}
+
 func BenchmarkLarge(b *testing.B) {
        b.StopTimer()
        b.ReportAllocs()