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

fix: stack-use-after-scope variable ordering #4355

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

jmayclin
Copy link
Contributor

Resolved issues:

#4354

Description of changes:

Co-authored-by: Lindsay Stewart <[email protected]>

When running with newer address sanitizers, ASAN detects a stack-use-after-scope variable ordering issue.

  This frame has 4 object(s):
    [32, 40) 'total_bytes' (line 422)
    [64, 88) 'new_bufs' (line 426)
    [128, 384) 'new_bufs_mem' (line 427) <== Memory access at offset 128 is inside this variable
    [448, 456) 'bytes_written' (line 433)

Sequence of events

  1. s2n_blob new_bufs is defined
  2. new_bufs_mem is defined
  3. function finishes
  4. new_bufs_mem goes out of scope first (because it was defined last)
  5. s2n_blob new_bufs goes out of scope, triggering the DEFER_CLEANUP
  6. this zeros the buffer of new_bufs, but that buffer (new_bufs_mem) is already out of scope

This commit switches the order of new_bufs and new_bufs_mem so that new_bufs_mem is still in scope when s2n_free_or_wipe accesses it.

Call-outs:

This failure does reproduce in #4048. build dashboard I cleaned up that PR and will be merging it after this fix is merged in.

Testing:

Confirmed on local host that ASAN errors no longer occur. I then rebased this commit on the ASAN commit branch and ran it under the new codebuild job to confirm that no errors were found.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jan 10, 2024
@jmayclin jmayclin marked this pull request as ready for review January 10, 2024 20:55
Copy link
Contributor

@camshaft camshaft 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 with Lindsay's change

Co-authored-by: Lindsay Stewart <[email protected]>
@jmayclin jmayclin enabled auto-merge (squash) January 10, 2024 21:37
@jmayclin jmayclin merged commit 35c9f18 into aws:main Jan 10, 2024
29 checks passed
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Jan 17, 2024
@jmayclin jmayclin deleted the stack-use-after-scope branch June 15, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants