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.

Sign up for GitHub

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

Jump to bottom

Limit manifest's change set size #490

Closed
schomatis opened this issue May 16, 2018 · 6 comments · Fixed by #1119
Closed

Limit manifest's change set size #490

schomatis opened this issue May 16, 2018 · 6 comments · Fixed by #1119
Labels
area/security Security related issues. kind/bug Something is broken. priority/P2 Somehow important but would not block a release. status/accepted We accept to investigate or work on it.
Milestone
Future ideas

Comments

@schomatis
Copy link
Contributor

(In a similar way as it was done for the value log replay.)

https://github.com/dgraph-io/badger/blob/dc0df253d239ff6abe3ffbbcdf3cb1e49e646369/manifest.go#L356-L357
@Stebalien Stebalien mentioned this issue May 16, 2018
Open
14 tasks
@manishrjain
Copy link
Contributor

Reasoning?

@schomatis
Copy link
Contributor Author

The same as the one described in vulnerability number 1 of the ALICE post which the cited PR fixes.

The vulnerability occurs when the key and value lengths in a value log entry header contain garbage data. These lengths are used to figure out how to read the key and value. A 4-byte integer’s largest value is ~4 billion, causing a worst case memory allocation of 8GB (4GB for each of the key and value). This can cause the process to go out of memory, resulting in Badger refusing to start.

@manishrjain
Copy link
Contributor

I see. Good catch.

@manishrjain manishrjain added the kind/maintenance Maintenance tasks, such as refactoring, with no impact in features. label May 16, 2018
@schomatis
Copy link
Contributor Author

What I would like to understand is why this wasn't caught with the (current and previous) Tove/ALICE tests.

@manishrjain
Copy link
Contributor

ALICE would have to see a lot of writes going into MANIFEST, which doesn't happen as often as writes which go into the value log. If run long enough, on a powerful machine, this might trigger, I reckon. There's a time gap between write and sync, a crash should happen at that time and must leave some garbage data appended to the file.

https://github.com/dgraph-io/badger/blob/dc0df253d239ff6abe3ffbbcdf3cb1e49e646369/manifest.go#L208-L215

@campoy campoy added area/security Security related issues. kind/bug Something is broken. priority/P1 Serious issue that requires eventual attention (can wait a bit) status/accepted We accept to investigate or work on it. and removed kind/maintenance Maintenance tasks, such as refactoring, with no impact in features. labels May 28, 2019
@campoy campoy added this to the Unplanned milestone May 28, 2019
@campoy
Copy link
Contributor

campoy commented May 29, 2019

This seems like a serious issue, so I'd like to have you @manishrjain check whether it's indeed something we should fix or it's not relevant to the current codebase anymore.

@campoy campoy added priority/P2 Somehow important but would not block a release. and removed priority/P1 Serious issue that requires eventual attention (can wait a bit) labels Jun 13, 2019
@campoy campoy modified the milestones: Unplanned, Future ideas Jun 13, 2019
@jarifibrahim jarifibrahim mentioned this issue Nov 13, 2019
Merged
jarifibrahim pushed a commit that referenced this issue Nov 20, 2019
Limit manifest's change set size (#1119)
5f94f68 
This PR limits the amount of memory we allocated for reading
the manifest changes set's size. When a manifest file is corrupted,
in the worst case we might end up allocating more than 4GB.

This PR ensures we don't over-allocate the byte slice.
Fixes #490
jarifibrahim pushed a commit that referenced this issue Mar 12, 2020
Limit manifest's change set size (#1119)
48aaa1c 
This PR limits the amount of memory we allocated for reading
the manifest changes set's size. When a manifest file is corrupted,
in the worst case we might end up allocating more than 4GB.

This PR ensures we don't over-allocate the byte slice.
Fixes #490

(cherry picked from commit 5f94f68)
manishrjain pushed a commit to outcaste-io/outserv that referenced this issue Jul 6, 2022
Limit manifest's change set size (#1119)
909ba37 
This PR limits the amount of memory we allocated for reading
the manifest changes set's size. When a manifest file is corrupted,
in the worst case we might end up allocating more than 4GB.

This PR ensures we don't over-allocate the byte slice.
Fixes hypermodeinc/badger#490
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/security Security related issues. kind/bug Something is broken. priority/P2 Somehow important but would not block a release. status/accepted We accept to investigate or work on it.
Milestone
Future ideas
Development

Successfully merging a pull request may close this issue.

Limit manifest's change set size hypermodeinc/badger
3 participants
@manishrjain @campoy @schomatis