Skip to content

Conversation

joshlf
Copy link
Member

@joshlf joshlf commented Feb 25, 2025

This commit makes the following improvements:

  • Removes the Inaccessible aliasing mode. This mode was not unsound,
    but it was unnecessary in practice.
  • Replaces Unknown with Unaligned for alignment.
  • Replaces Unknown with Uninit for validity.

Finally, with the exception of transparent_wrapper_into_inner, this
commit ensures that all Ptr methods which modify the type or validity
of a Ptr are sound.

Previously, we modeled validity as "knowledge" about the Ptr's
referent (see #1866 for a more in-depth explanation). In particular, we
assumed that it was always sound to "forget" information about a Ptr's
referent in the same way in which it is sound to "forget" that a Ptr
is validly-aligned, converting a Ptr<T, (_, Aligned, _)> to a
Ptr<T, (_, Unaligned, _)>.

The problem with this approach is that validity doesn't just specify
what bit patterns can be expected to be read from a Ptr's referent, it
also specifies what bit patterns are permitted to be written to the
referent. Thus, "forgetting" about validity and expanding the set of
expected bit patterns also expands the set of bit patterns which can be
written.

Consider, for example, "forgetting" that a Ptr<bool, (_, _, Valid)> is
bit-valid, and downgrading it to a Ptr<bool, (_, _, Initialized)>. If
the aliasing is Exclusive, then the resulting Ptr would permit
writing arbitrary u8s to the referent, violating the bit validity of
the bool.

This commit moves us in the direction of a new model, in which changes
to the referent type or the validity of a Ptr must abide by the
following rules:

  • If the resulting Ptr permits mutation of its referent (either via
    interior mutation or Exclusive aliasing), then the set of allowed
    bit patterns in the referent may not expand
  • If the original Ptr permits mutation of its referent while the
    resulting Ptr is also live (i.e., via interior mutation on a
    Shared Ptr), then the set of allowed bit patterns in the referent
    may not shrink

This commit does not fix transparent_wrapper_into_inner, which will
require an overhaul or replacement of the TransparentWrapper trait.

Makes progress on #2226, #1866

Co-authored-by: Jack Wrenn jswrenn@amazon.com


This PR is on branch v0.8.x-ptr-invariant-mapping.

@joshlf joshlf force-pushed the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch 2 times, most recently from a60c4a8 to d603a4f Compare February 25, 2025 23:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 93.20388% with 14 lines in your changes missing coverage. Please review.

Project coverage is 87.48%. Comparing base (4173443) to head (fb585cd).

Files with missing lines Patch % Lines
src/util/macro_util.rs 74.07% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2397      +/-   ##
==========================================
- Coverage   87.82%   87.48%   -0.35%     
==========================================
  Files          17       17              
  Lines        6200     6279      +79     
==========================================
+ Hits         5445     5493      +48     
- Misses        755      786      +31     

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

@joshlf joshlf force-pushed the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch 3 times, most recently from d6f0cd0 to 6065dd6 Compare February 26, 2025 21:16
@joshlf joshlf changed the title [WIP] STUFF [pointer] Improve soundness of invariant modeling Feb 26, 2025
@joshlf joshlf requested a review from jswrenn February 26, 2025 21:17
@joshlf joshlf force-pushed the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch 2 times, most recently from 031279e to c8eff72 Compare February 26, 2025 21:18
@joshlf joshlf force-pushed the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch from c8eff72 to 6640f2a Compare February 27, 2025 19:12
@joshlf joshlf enabled auto-merge February 27, 2025 19:12
This commit makes the following improvements:
- Removes the `Inaccessible` aliasing mode. This mode was not unsound,
  but it was unnecessary in practice.
- Replaces `Unknown` with `Unaligned` for alignment.
- Replaces `Unknown` with `Uninit` for validity.

Finally, with the exception of `transparent_wrapper_into_inner`, this
commit ensures that all `Ptr` methods which modify the type or validity
of a `Ptr` are sound.

Previously, we modeled validity as "knowledge" about the `Ptr`'s
referent (see #1866 for a more in-depth explanation). In particular, we
assumed that it was always sound to "forget" information about a `Ptr`'s
referent in the same way in which it is sound to "forget" that a `Ptr`
is validly-aligned, converting a `Ptr<T, (_, Aligned, _)>` to a
`Ptr<T, (_, Unaligned, _)>`.

The problem with this approach is that validity doesn't just specify
what bit patterns can be expected to be read from a `Ptr`'s referent, it
also specifies what bit patterns are permitted to be *written* to the
referent. Thus, "forgetting" about validity and expanding the set of
expected bit patterns also expands the set of bit patterns which can be
written.

Consider, for example, "forgetting" that a `Ptr<bool, (_, _, Valid)>` is
bit-valid, and downgrading it to a `Ptr<bool, (_, _, Initialized)>`. If
the aliasing is `Exclusive`, then the resulting `Ptr` would permit
writing arbitrary `u8`s to the referent, violating the bit validity of
the `bool`.

This commit moves us in the direction of a new model, in which changes
to the referent type or the validity of a `Ptr` must abide by the
following rules:
- If the resulting `Ptr` permits mutation of its referent (either via
  interior mutation or `Exclusive` aliasing), then the set of allowed
  bit patterns in the referent may not expand
- If the original `Ptr` permits mutation of its referent while the
  resulting `Ptr` is also live (i.e., via interior mutation on a
  `Shared` `Ptr`), then the set of allowed bit patterns in the referent
  may not shrink

This commit does not fix `transparent_wrapper_into_inner`, which will
require an overhaul or replacement of the `TransparentWrapper` trait.

Makes progress on #2226, #1866

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
gherrit-pr-id: I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93
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.

Reviewed in-person.

@joshlf joshlf force-pushed the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch from 6640f2a to fb585cd Compare February 27, 2025 19:13
@joshlf joshlf added this pull request to the merge queue Feb 27, 2025
Merged via the queue into main with commit 367e68b Feb 27, 2025
87 checks passed
@joshlf joshlf deleted the I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93 branch February 27, 2025 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants