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

Fix alignment errors in hashtable fuzzer #24276

Closed
wants to merge 1 commit into from
Closed

Fix alignment errors in hashtable fuzzer #24276

wants to merge 1 commit into from

Conversation

nhorman
Copy link
Contributor

@nhorman nhorman commented Apr 26, 2024

we extract several values (uint16_t and uint64_t from the fuzzer buff passed in, but they weren't aligned on 2 and 8 byte boundaries. Adjust the fuzzer to use the proper offsets

Fixes #24272

@nhorman nhorman self-assigned this Apr 26, 2024
@nhorman nhorman added the branch: master Merge to master branch label Apr 26, 2024
@davidben
Copy link
Contributor

I think a better fix would be to load the values via memcpy:

  • You don't waste the fuzzer's time mutating ignored bytes in the padding
  • Aligned or not, this is still a strict aliasing violation
  • This assumes the start of the buffer is aligned and I'm not sure if libFuzzer actually guarantees this.

@nhorman
Fix alignment errors in hashtable fuzzer
f6b270b 
we extract several values (uint16_t and uint64_t from the fuzzer buff
passed in, but they weren't aligned on 2 and 8 byte boundaries.  Adjust
the fuzzer to memcpy data to the target variables to avoid unalignment
issues

Fixes openssl#24272
@nhorman
Copy link
Contributor Author

nhorman commented Apr 26, 2024

updated to use memcpy

@nhorman nhorman added severity: urgent Fixes an urgent issue (exempt from 24h grace period) severity: regression The issue/pr is a regression from previous released version approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Apr 26, 2024
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Apr 28, 2024
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM; memcpy() is also understood by clang and gcc as an idiom to load potentially unaligned data, and will be optimised to a single load instruction on platforms where this is possible

@tom-cosgrove-arm tom-cosgrove-arm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 29, 2024
@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Apr 29, 2024
@t8m
Copy link
Member

t8m commented Apr 29, 2024

@tom-cosgrove-arm @paulidale Do you agree with urgent?

Sashan
Sashan approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@Sashan Sashan 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. change fixes all those places in hastable.c

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@tom-cosgrove-arm
Copy link
Contributor

@t8m Apologies, I've been AFK, but yes, agree with urgent (although a bit late now!)

@tom-cosgrove-arm tom-cosgrove-arm added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 30, 2024
@t8m
Copy link
Member

t8m commented Apr 30, 2024

Merged to the master branch. Thank you.

@t8m t8m closed this Apr 30, 2024
openssl-machine pushed a commit that referenced this pull request Apr 30, 2024
@nhorman @t8m
Fix alignment errors in hashtable fuzzer
c04901b 
we extract several values (uint16_t and uint64_t from the fuzzer buff
passed in, but they weren't aligned on 2 and 8 byte boundaries.  Adjust
the fuzzer to memcpy data to the target variables to avoid unalignment
issues

Fixes #24272

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #24276)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sashan Sashan Sashan approved these changes

@paulidale paulidale paulidale approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@t8m t8m Awaiting requested review from t8m

@levitte levitte Awaiting requested review from levitte

@hlandau hlandau Awaiting requested review from hlandau

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

Assignees

@nhorman nhorman

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: regression The issue/pr is a regression from previous released version severity: urgent Fixes an urgent issue (exempt from 24h grace period) triaged: bug The issue/pr is/fixes a bug
Projects
Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI regression from the new hashtable support
7 participants
@nhorman @davidben @t8m @openssl-machine @tom-cosgrove-arm @Sashan @paulidale