Skip to content

Comments

Fix soundness of FromBytes::read_from_io#2320

Merged
joshlf merged 1 commit intogoogle:mainfrom
kupiakos:fromzeros-soundness-0.9
Feb 19, 2025
Merged

Fix soundness of FromBytes::read_from_io#2320
joshlf merged 1 commit intogoogle:mainfrom
kupiakos:fromzeros-soundness-0.9

Conversation

@kupiakos
Copy link
Contributor

@kupiakos kupiakos commented Feb 7, 2025

See #2319.

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.31%. Comparing base (79ec7c4) to head (dad322d).

Files with missing lines Patch % Lines
src/lib.rs 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2320      +/-   ##
==========================================
- Coverage   89.32%   89.31%   -0.02%     
==========================================
  Files          16       16              
  Lines        6025     6045      +20     
==========================================
+ Hits         5382     5399      +17     
- Misses        643      646       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Feb 7, 2025
kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Feb 7, 2025
@kupiakos kupiakos force-pushed the fromzeros-soundness-0.9 branch 2 times, most recently from 867129d to a6bb505 Compare February 8, 2025 00:05
kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Feb 8, 2025
See google#2319. Backport of google#2320.
Includes a Miri test confirming the previous unsoundness.
kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Feb 8, 2025
See google#2319. Backport of google#2320.
Includes a Miri test confirming the previous unsoundness.
@kupiakos kupiakos force-pushed the fromzeros-soundness-0.9 branch from a6bb505 to 9ab80be Compare February 8, 2025 00:09
@kupiakos
Copy link
Contributor Author

kupiakos commented Feb 8, 2025

I've confirmed that the new test fails Miri when run without the change in read_from_io

Copy link
Collaborator

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. We really ought to update FromZeros::new_zeroed to match the disclaimers of MaybeUninit::zeroed, but that's a distinct issue from the one you're fixing and can be handled in a follow-up PR.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for catching this! Two small comment nits, but otherwise looks good.

@joshlf joshlf force-pushed the fromzeros-soundness-0.9 branch from 0a9c7b6 to ce321e2 Compare February 19, 2025 13:20
@joshlf joshlf enabled auto-merge February 19, 2025 13:20
See google#2319. Includes a Miri test confirming the previous unsoundness.

gherrit-pr-id: Iede94c196c710c74d970c93935f1539e07446e50
@joshlf joshlf force-pushed the fromzeros-soundness-0.9 branch from ce321e2 to dad322d Compare February 19, 2025 13:21
@joshlf joshlf added this pull request to the merge queue Feb 19, 2025
google-pr-creation-bot pushed a commit to google-pr-creation-bot/zerocopy that referenced this pull request Feb 19, 2025
See google#2319. Includes a Miri test confirming the previous unsoundness.

gherrit-pr-id: Iede94c196c710c74d970c93935f1539e07446e50
@joshlf
Copy link
Member

joshlf commented Feb 19, 2025

Backporting to 0.8 in #2358

Merged via the queue into google:main with commit 1339ee9 Feb 19, 2025
69 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2025
See #2319. Includes a Miri test confirming the previous unsoundness.

gherrit-pr-id: Iede94c196c710c74d970c93935f1539e07446e50

Co-authored-by: Alyssa Haroldsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants