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 provenance issues #100

Merged
merged 5 commits into from
Jun 11, 2022
Merged

Fix provenance issues #100

merged 5 commits into from
Jun 11, 2022

Conversation

Noratrieb
Copy link
Contributor

@Noratrieb Noratrieb commented May 28, 2022

When creating a BoxString using from_string or from_box_str, str::as_ptr was used to get the pointer, which only has read provenance for the initialized part of the string. Going through Vec in the from_string and through Box::into_raw in the from_box_str case fixed the issue.

Using proper miri flags (MIRIFLAGS=-Zmiri-strict-provenance) miri also complains about repr::arc, which is wrong since it uses a zero length array to be a fake custom dst, which is not allowed.

When creating a `BoxString` using `from_string` or `from_box_str`,
`str::as_ptr` was used to get the pointer, which only has read
provenance for the initialized part of the string. Going through `Vec`
in the `from_string` and through `Box::into_raw` in the `from_box_str`
case fixed the issue.
@Noratrieb Noratrieb changed the title Fix BoxedString not having the right provenance Fix provenance issues May 28, 2022
Before, it used a zero length array to signal it being a fake DST.
This is not allowed, since it doesn't allow writing past the end
of this header. Rewrite the entire module to use an `ArcStringPtr`
abstraction instead, which allows to easily write to the buffer
and still have the header accessible.
@Noratrieb
Copy link
Contributor Author

The MSRV check fails because of addr_of_mut, which was added in 1.51.
There are 3 options to fix this:

  • Raise the MSRV
  • Create a reference (unsound)
  • Do manual pointer arithmetic since it's repr(C)

Copy link
Owner

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! Just see the one comment around deleting the arc module since it's not used anymore, after that we should be good to go!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
compact_str/src/repr/arc/mod.rs Outdated Show resolved Hide resolved
compact_str/src/repr/boxed/mod.rs Show resolved Hide resolved
Noratrieb and others added 3 commits June 10, 2022 21:18
Before, the miriflags didn't work correctly because they had a typo.
Fix the typo, and also use the new strict-provenance option which
implies the two used before.
The module is unused and can therefore be removed.
@ParkMyCar ParkMyCar self-requested a review June 10, 2022 23:36
Copy link
Owner

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@ParkMyCar ParkMyCar merged commit cd6ea95 into ParkMyCar:main Jun 11, 2022
@Noratrieb Noratrieb deleted the fix-provenance-issues branch June 11, 2022 07:24
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.

2 participants