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

sstable/rowblk: block restarts overflow #4216

Open
Shifu-Liu-tikkala opened this issue Dec 30, 2024 · 3 comments
Open

sstable/rowblk: block restarts overflow #4216

Shifu-Liu-tikkala opened this issue Dec 30, 2024 · 3 comments

Comments

@Shifu-Liu-tikkala
Copy link

Shifu-Liu-tikkala commented Dec 30, 2024

I have seen a panic issue on pebble Db on one of my Ethereum nodes. After digging into the issue found the reason is that when blockIter.restarts > 2G which will became a negative value(int32), in the function SeekGE() it will use int32 to read the offset. Due to the negative offset it caused a out-of-bound read panic.
I modified the code and use uint32 instead of int32, it works well after the testing.

--- /local/github/[email protected]/sstable/block.go 2024-12-27 04:05:56.991447085 +0000
+++ sstable/block.go    2024-12-30 01:41:21.819025553 +0000
@@ -707,7 +707,7 @@
                for index < upper {
                        h := int32(uint(index+upper) >> 1) // avoid overflow when computing h
                        // index ≤ h < upper
-                       offset := decodeRestart(i.data[i.restarts+4*h:])
+                       offset := decodeRestart(i.data[uint32(i.restarts)+uint32(4*h):])
                        // For a restart point, there are 0 bytes shared with the previous key.
                        // The varint encoding of 0 occupies 1 byte.
                        ptr := unsafe.Pointer(uintptr(i.ptr) + uintptr(offset+1))

Jira issue: PEBBLE-317

@jbowens
Copy link
Collaborator

jbowens commented Dec 30, 2024

Thanks for the report; we'll fix. I went stepping through our issue backlog and found #3333 which seems to be a duplicate of this issue. When we fix it, we should close out both.


Cockroach folks, I'm thinking Edward will pick this up in the next couple weeks.

@jbowens jbowens changed the title OutOfBound panic if blockIter.restarts > 2G sstable: block restarts overflow Dec 30, 2024
@jbowens
Copy link
Collaborator

jbowens commented Dec 30, 2024

We'll want to fix this on master, the crl-release branches and the non-crl release branches. Unfortunately it'll probably mean making the changes multiple times given the amount of code drift.

@jbowens jbowens changed the title sstable: block restarts overflow sstable/rowblk: block restarts overflow Dec 30, 2024
@Shifu-Liu-tikkala
Copy link
Author

Thanks for the report; we'll fix. I went stepping through our issue backlog and found #3333 which seems to be a duplicate of this issue. When we fix it, we should close out both.

Cockroach folks, I'm thinking Edward will pick this up in the next couple weeks.

Yes it seems like the exactly same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Incoming
Development

No branches or pull requests

3 participants