Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2.EncodeBetter reads out of bounds and panics #290

Closed
greatroar opened this issue Oct 27, 2020 · 4 comments · Fixed by #291
Closed

s2.EncodeBetter reads out of bounds and panics #290

greatroar opened this issue Oct 27, 2020 · 4 comments · Fixed by #291

Comments

@greatroar
Copy link
Contributor

When I run

package main

import (
	"io/ioutil"
	"log"
	"os"

	"github.com/klauspost/compress/s2"
)

func main() {
	p, err := ioutil.ReadFile(os.Args[1])
	if err != nil {
		log.Fatal(err)
	}
	p = p[:len(p):len(p)]

	s2.EncodeBetter(nil, p)
}

on the attached file, I get

panic: runtime error: slice bounds out of range [:8] with capacity 6

goroutine 1 [running]:
github.com/klauspost/compress/s2.load64(...)
	/home/me/go/pkg/mod/github.com/klauspost/compress@v1.11.1/s2/encode_all.go:22
github.com/klauspost/compress/s2.encodeBlockBetter(0xc00011c003, 0x10011, 0x10011, 0xc00010a000, 0x1000d, 0x1000d, 0x0)
	/home/me/go/pkg/mod/github.com/klauspost/compress@v1.11.1/s2/encode_better.go:186 +0xc68
github.com/klauspost/compress/s2.EncodeBetter(0xc00011c000, 0x10014, 0x10014, 0xc00010a000, 0x1000d, 0x1000d, 0x0, 0xc000014118, 0x0)
	/home/me/go/pkg/mod/github.com/klauspost/compress@v1.11.1/s2/encode.go:93 +0x20d
main.main()
	/home/me/tmp/tests2.go:18 +0xd8
exit status 2

It looks like some bounds check is missing. The funny thing is that the program doesn't crash if I omit the p = p[:len(p):len(p)], but the program were this crash first occurred didn't pull that trick.

@klauspost
Copy link
Owner

That is indeed strange. I will take a look.

@greatroar
Copy link
Contributor Author

greatroar commented Oct 27, 2020

Oh right, the difference between p[:len(p):len(p)] and not doing that is obvious. s2.load64 tries to force BCE, but by doing do actually extends the slice. I can see that the original program also "worked" until the slice's length was too close to its capacity.

@klauspost
Copy link
Owner

Yeah. Found the issue. Just adding a few tests.

@greatroar
Copy link
Contributor Author

Thanks!

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 a pull request may close this issue.

2 participants