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

store/internal: validate keys before calling ProofsFromMap #9235

Merged
merged 1 commit into from
May 2, 2021
Merged

store/internal: validate keys before calling ProofsFromMap #9235

merged 1 commit into from
May 2, 2021

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Apr 30, 2021 •
edited
Loading

Description

Otherwise, an empty key as input or present in data can cause a panic at
runtime.

Caught by oss-fuzz: https://oss-fuzz.com/testcase-detail/4647668077953024

Fixes #9233

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 30, 2021

cc @odeke-em

@codecov
Copy link

codecov bot commented Apr 30, 2021 •
edited
Loading

Codecov Report

Merging #9235 (e2b24ed) into master (72873a0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head e2b24ed differs from pull request most recent head e30934b. Consider uploading reports for the commit e30934b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9235      +/-   ##
==========================================
+ Coverage   60.12%   60.14%   +0.01%     
==========================================
  Files         595      595              
  Lines       37188    37194       +6     
==========================================
+ Hits        22361    22369       +8     
+ Misses      12847    12846       -1     
+ Partials     1980     1979       -1
Impacted Files Coverage Δ
store/internal/proofs/create.go 81.39% <100.00%> (+13.82%) ⬆️
snapshots/store.go 74.31% <0.00%> (-1.10%) ⬇️

tac0turtle
tac0turtle approved these changes Apr 30, 2021
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

lgtm

@tac0turtle
Copy link
Member

could you add a changelog please?

odeke-em
odeke-em requested changes Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you for this change @cuonglm! LGTM, but there is no reason we need to incur the []byte->string conversion when we can use len(key) == 0

store/internal/proofs/create.go Outdated Show resolved Hide resolved
store/internal/proofs/create.go Outdated Show resolved Hide resolved
alexanderbez
alexanderbez previously requested changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Looks good, but lets accept @odeke-em's requested changes.

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 30, 2021

could you add a changelog please?

Done

Thank you for this change @cuonglm! LGTM, but there is no reason we need to incur the []byte->string conversion when we can use len(key) == 0

Done

robert-zaremba
robert-zaremba approved these changes Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

utACK

odeke-em
odeke-em approved these changes Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @cuonglm!

@robert-zaremba robert-zaremba added backport/0.42.x (Stargate) A:automerge Automatically merge PR once all prerequisites pass. C:Store labels May 1, 2021
@robert-zaremba
Copy link
Collaborator

robert-zaremba commented May 1, 2021 •
edited
Loading

@alexanderbez - could you release your block on this PR? Everything was addressed.

@cuonglm
store/internal: validate keys before calling ProofsFromMap
e30934b 
Otherwise, an empty key as input or present in data can cause a panic at
runtime.

Caught by oss-fuzz: https://oss-fuzz.com/testcase-detail/4647668077953024

Fixes #9233
@odeke-em odeke-em dismissed alexanderbez’s stale review May 2, 2021 22:53

Addressed. Thank you Alexander.

@odeke-em odeke-em merged commit 711976e into cosmos:master May 2, 2021
@odeke-em odeke-em deleted the cuonglm/issue-9233 branch May 2, 2021 22:54
mergify bot pushed a commit that referenced this pull request May 2, 2021
store/internal: validate keys before calling ProofsFromMap (#9235)
51737da 
Otherwise, an empty key as input or present in data can cause a panic at
runtime.

Caught by oss-fuzz: https://oss-fuzz.com/testcase-detail/4647668077953024

Fixes #9233

(cherry picked from commit 711976e)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request May 2, 2021
Merged
alessio pushed a commit that referenced this pull request May 6, 2021
@mergify
store/internal: validate keys before calling ProofsFromMap (backport #…
81b1049 
…9235) (#9247)

Fixes #9233

(cherry picked from commit 711976e)
Co-authored-by: Cuong Manh Le <cuong@orijtech.com>
Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@odeke-em odeke-em odeke-em approved these changes

@robert-zaremba robert-zaremba robert-zaremba approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@amaury1093 amaury1093 Awaiting requested review from amaury1093

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

store/internal/proofs: CreateNonMembershipProof/CreateMembershipProof panic with empty key
5 participants
@cuonglm @tac0turtle @robert-zaremba @odeke-em @alexanderbez