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

OOB read using ReadFrom #85

Closed
klauspost opened this issue Sep 5, 2022 · 5 comments
Closed

OOB read using ReadFrom #85

klauspost opened this issue Sep 5, 2022 · 5 comments

Comments

@klauspost
Copy link
Contributor

When upgrading to latest version we observe this error:

panic: runtime error: index out of range [14977] with length 14977

goroutine 761 [running]:
github.com/bits-and-blooms/bitset.(*BitSet).ReadFrom(0xc00036ff00, {0x50e95a0?, 0xc0003b8dc0?})
	github.com/bits-and-blooms/[email protected]/bitset.go:955 +0x392
github.com/bits-and-blooms/bloom/v3.(*BloomFilter).ReadFrom(0xc001096a50, {0x50e95a0, 0xc0003b8dc0})
	github.com/bits-and-blooms/bloom/[email protected]/bloom.go:367 +0xd6
github.com/minio/minio/cmd.(*dataUpdateTracker).deserialize(0xc000eb4850, {0x50e95a0, 0xc0003b8dc0}, {0x1b?, 0xc001344f00?, 0x0?})
	github.com/minio/minio/cmd/data-update-tracker.go:434 +0x487
github.com/minio/minio/cmd.(*dataUpdateTracker).load(0xc000eb4850, {0x50fa338, 0xc000ff7280}, {0xc001254700?, 0x64, 0xc000128c80?})
[...]

We are de-serializing into a new instance of &bloom.BloomFilter{}.

Possible regression from bits-and-blooms/bitset#103

@thanhpk
Copy link

thanhpk commented Sep 7, 2022

How can I reproduce this?

@klauspost
Copy link
Contributor Author

klauspost commented Sep 7, 2022

@thanhpk I found the bug. You are over-reading. Upgrading from bitset v1.2.0 you changed it to a read loop to:

	for i := uint64(0); i < length; i++ {
		if _, err := reader.Read(item); err != nil {
			if err == io.EOF {
				break // done
			}
			return 0, err
		}
		newset.set[i] = binaryOrder.Uint64(item)
	}

However length is the number of bits, not the size of newset.set[i].

If the stream ends, it will exit "early" from the loop due to io.EOF, but if there is more data it will read too much and crash.

It would seem like length = uint64(wordsNeeded(uint(length))) is needed. But probably you also detect if all data you expect has been read and return io.ErrUnexpectedEOF if not.

@klauspost
Copy link
Contributor Author

klauspost commented Sep 7, 2022

(feel free to transfer the issue to github.com/bits-and-blooms/bitset if you see fit). Problem is indeed from bits-and-blooms/bitset#103

@thanhpk
Copy link

thanhpk commented Sep 7, 2022

great catch, thanks. I submited a pull request bits-and-blooms/bitset#107.

@lemire
Copy link
Member

lemire commented Sep 7, 2022

Closed via merging #107

@lemire lemire closed this as completed Sep 7, 2022
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

No branches or pull requests

3 participants