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

Add safeguards to bbolt CLI tool #354

Merged
merged 1 commit into from
Dec 18, 2022
Merged

Add safeguards to bbolt CLI tool #354

merged 1 commit into from
Dec 18, 2022

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Dec 15, 2022

  • perform sanity check whether pages being fetched are bbolt pages and not random data (e.g. they self identify using proper magic number of 'pageid' field.
  • protect against alocation of extreeme emount of RAM if 'overflow pages' field contains too much data.

Signed-off-by: Piotr Tabor ptab@google.com

@ptabor ptabor mentioned this pull request Dec 15, 2022
Merged
ahrtr
ahrtr reviewed Dec 16, 2022
View reviewed changes
cmd/bbolt/main.go Outdated
overflowN := p.overflow
if overflowN >= uint32(hwm) {
Copy link
Member

@ahrtr ahrtr Dec 16, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to exclude the 2 meta pages and current page. For example, assuming there are 6 pages in total, then the overflowN field of any page should be <= 3

Suggested change
if overflowN >= uint32(hwm) {
if overflowN > uint32(hwm) - 3 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected. I didn't intended to be that precise - just protect OS from extensive allocation.

@ptabor
Safety check to 'bbolt page' command.
c34493c 
Signed-off-by: Piotr Tabor <ptab@google.com>
@ptabor ptabor mentioned this pull request Dec 17, 2022
Closed
ahrtr
ahrtr approved these changes Dec 17, 2022
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ptabor ptabor merged commit bad964e into etcd-io:master Dec 18, 2022
@ahrtr
Copy link
Member

ahrtr commented Dec 21, 2022

could you please add a changelog item in CHANGELOG.md?

@ahrtr ahrtr added this to the 1.3.7 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

@serathius serathius Awaiting requested review from serathius

Assignees
No one assigned
Labels
None yet
Milestone
1.3.7
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ptabor @ahrtr