Document unsafe blocks in core::{cell, str, sync} #66564

foeb · 2019-11-20T02:42:04Z

Split from #66506 (issue #66219). Hopefully doing a chunk at a time is more manageable!

rust-highfive · 2019-11-20T02:42:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @RalfJung (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

RalfJung · 2019-11-20T09:57:31Z

Cc @rust-lang/wg-unsafe-code-guidelines

RalfJung · 2019-11-20T09:57:57Z

There are also still some open comments at #66506

JohnCSimon · 2019-11-24T02:20:54Z

Ping from triage
@foeb can you address the comments from PR #66506 as per @RalfJung ?
#66564 (comment)

JohnCSimon · 2019-11-30T06:43:42Z

Pinging again from triage:
@foeb can you address the comments in this PR or post your status if this ready for review?

foeb · 2019-11-30T18:24:08Z

@JohnCSimon I've addressed the comments as much as I can without further comments from the reviewer.

JohnCSimon · 2019-12-14T02:45:03Z

Ping from triage:
@RalfJung can you comment on this?

RalfJung · 2019-12-14T10:18:59Z

My status is that I still don't know when I will have the time to look at this -- sorry. With the holidays coming up, my free time has diminished to basically zero.

RalfJung · 2019-12-22T15:20:28Z

src/libcore/cell.rs

@@ -369,6 +367,7 @@ impl<T> Cell<T> {
        if ptr::eq(self, other) {
            return;
        }
+        // SAFETY: not threadsafe, but it's OK since we know `Cell` isn't threadsafe


Besides data races, a major concern here is invalidating pointers. The reason this is safe is that Cell rules out interior pointers -- there can be nothing pointing into either of these Cell, so we can swap out their content just fine.

This applies to the other unsafe blocks in this file as well.

Also, "not threadsafe is okay" is negative reasoning, that's somewhat backwards. What we need here is positive evidence that there cannot be a race, and that evidence is that Cell is !Sync. I think that's what you mean by your second use of the word "threadsafe" but it's not entirely clear.

RalfJung

So sorry for the long delay. I expected this to be huge amounts of work so I pushed it back again and again to start the review... turns out I hardly know most of this code so I can't intelligently review it anyway. :/

This mostly looks good to me, modulo the comments I left. However I had to skip most of the str module as I am entirely unfamiliar with that code. Could someone from @rust-lang/libs check that part?

src/libcore/cell.rs

src/libcore/str/lossy.rs

src/libcore/str/mod.rs

RalfJung · 2019-12-22T15:32:35Z

src/libcore/str/mod.rs

@@ -1538,6 +1548,9 @@ fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> {
            if align != usize::max_value() && align.wrapping_sub(index) % usize_bytes == 0 {
                let ptr = v.as_ptr();
                while index < blocks_end {
+                    // SAFETY: since `align - index` and `ascii_block_size` are multiples of
+                    // `usize_bytes`, `ptr.add(index)` is always aligned with a `usize` so we
+                    // may cast directly to a `const` pointer.


Casting to a raw const ptr is always possible so I a somewhat puzzled by this comment. Also, why does this implication hold? Not sure how you are going from align-index to `ptr.add(index)´.

And it's not really align-index anyway, it's wrapping_sub, but add must not overflow. So why does this all fit together?^^ (Probably this should be reviewed by someone who has seen this code before...)

Yeah, this is one part where I'm still unsure that it's correct, even after reading through the code a dozen times. I'd appreciate hearing why this works from someone who's familiar with the code, too.

This took me a while to understand, but basically:

align is an offset in the string at which point ptr.add(align) is guaranteed to be usize-aligned.

If align - index is a multiple of usize_bytes then ptr.add(index) is usize-aligned.

src/libcore/sync/atomic.rs

bors · 2019-12-23T05:51:38Z

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2019-12-30T18:41:56Z

Picking a @rust-lang/libs reviewer (not sure if team pings are checked by anyone)... r? @Amanieu. See #66564 (review) for my own review results.

Amanieu · 2019-12-31T15:07:53Z

This looks good to me (minus that one comment), but needs to be rebased.

Dylan-DPC-zz · 2020-01-16T09:36:32Z

@foeb can you rebase this? thanks

Co-Authored-By: Ralf Jung <post@ralfj.de>

rust-highfive · 2020-01-17T03:09:40Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-17T02:52:04.1781917Z ========================== Starting Command Output ===========================
2020-01-17T02:52:04.1790461Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/5c3b1c35-2c72-4997-8c6d-880fe7db84e0.sh
2020-01-17T02:52:04.1790834Z 
2020-01-17T02:52:04.1795244Z ##[section]Finishing: Disable git automatic line ending conversion
2020-01-17T02:52:04.1802620Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/66564/merge to s
2020-01-17T02:52:04.1804645Z Task         : Get sources
2020-01-17T02:52:04.1804683Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-17T02:52:04.1804720Z Version      : 1.0.0
2020-01-17T02:52:04.1804821Z Author       : Microsoft
---
2020-01-17T02:52:05.1571720Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-17T02:52:05.1582993Z ##[command]git config gc.auto 0
2020-01-17T02:52:05.1585988Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-17T02:52:05.1588346Z ##[command]git config --get-all http.proxy
2020-01-17T02:52:05.1595646Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66564/merge:refs/remotes/pull/66564/merge
---
2020-01-17T02:58:03.1260176Z Done!
2020-01-17T02:58:03.1263375Z some tidy checks failed
2020-01-17T02:58:03.1267180Z 
2020-01-17T02:58:03.1267530Z 
2020-01-17T02:58:03.1268823Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-01-17T02:58:03.1269679Z 
2020-01-17T02:58:03.1269907Z 
2020-01-17T02:58:03.1273894Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-01-17T02:58:03.1274251Z Build completed unsuccessfully in 0:01:28
2020-01-17T02:58:03.1274251Z Build completed unsuccessfully in 0:01:28
2020-01-17T02:58:03.1329383Z == clock drift check ==
2020-01-17T02:58:03.1339273Z   local time: Fri Jan 17 02:58:03 UTC 2020
2020-01-17T02:58:03.6589962Z   network time: Fri, 17 Jan 2020 02:58:03 GMT
2020-01-17T02:58:03.6597734Z == end clock drift check ==
2020-01-17T02:58:04.4052504Z 
2020-01-17T02:58:04.4179189Z ##[error]Bash exited with code '1'.
2020-01-17T02:58:04.4194096Z ##[section]Finishing: Run build
2020-01-17T02:58:04.4210705Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/66564/merge to s
2020-01-17T02:58:04.4212833Z Task         : Get sources
2020-01-17T02:58:04.4212914Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-17T02:58:04.4212992Z Version      : 1.0.0
2020-01-17T02:58:04.4213038Z Author       : Microsoft
2020-01-17T02:58:04.4213038Z Author       : Microsoft
2020-01-17T02:58:04.4213117Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-01-17T02:58:04.4213176Z ==============================================================================
2020-01-17T02:58:04.8114761Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-01-17T02:58:04.8154757Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/66564/merge to s
2020-01-17T02:58:04.8252896Z Cleaning up task key
2020-01-17T02:58:04.8253637Z Start cleaning up orphan processes.
2020-01-17T02:58:04.8400722Z Terminate orphan process: pid (6512) (python)
2020-01-17T02:58:04.8566945Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

foeb · 2020-01-17T06:19:10Z

@Dylan-DPC Thanks for the ping! I've rebased the branch and it should be ready to be merged.

Dylan-DPC-zz · 2020-01-17T07:11:18Z

@bors r=Amanieu

bors · 2020-01-17T07:11:19Z

📌 Commit 022a7de has been approved by Amanieu

bors · 2020-01-17T07:46:56Z

⌛ Testing commit 022a7de with merge 2ea51420ade2ad385c4950a1faa919da73f0a812...

Dylan-DPC-zz · 2020-01-17T09:17:13Z

@bors retry

(doing this to yield to a rollup retry )

@RalfJung

…l-str, r=Amanieu Document unsafe blocks in core::{cell, str, sync} Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable! r? @RalfJung

@RalfJung

…l-str, r=Amanieu Document unsafe blocks in core::{cell, str, sync} Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable! r? @RalfJung

@ghost

Rollup of 4 pull requests Successful merges: - #66564 (Document unsafe blocks in core::{cell, str, sync}) - #67791 (Implement Lift using interners instead of in_arena) - #68278 ([self-profiler] Add example to `-Z help` to turn on query key recording) - #68300 (Allow added string.insert benchmarks to compile) Failed merges: r? @ghost

bors · 2020-01-17T19:33:45Z

⌛ Testing commit 022a7de with merge 689fca0...

RalfJung · 2020-09-10T07:52:37Z

src/libcore/str/lossy.rs

@@ -17,6 +15,7 @@ impl Utf8Lossy {
    }

    pub fn from_bytes(bytes: &[u8]) -> &Utf8Lossy {
+        // SAFETY: Both use the same memory layout, and UTF-8 correctness isn't required.


FWIW, they actually are not guaranteed to use the same memory layout -- Utf8Lossy has no repr annotations, so layout is unspecified. In this case I think adding repr(transparent) to Utf8Lossy is enough.

If you know of more such cases, it would be great if you could add them in rust-lang/unsafe-code-guidelines#90. :)

Correct safety reasoning in `str::make_ascii_{lower,upper}case()` I don't understand why the previous comment was used (it was inserted in rust-lang#66564), but it doesn't explain why these functions are safe, only why `str::as_bytes{_mut}()` are safe. If someone thinks they make perfect sense, I'm fine with closing this PR.

rust-highfive assigned RalfJung Nov 20, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2019

foeb mentioned this pull request Nov 20, 2019

Document unsafe in libcore #66506

Closed

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2019

RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 4, 2019

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2019

RalfJung reviewed Dec 22, 2019

View reviewed changes

foeb force-pushed the 66219-document-unsafe-sync-cell-str branch from 5e02e58 to 9f4f19a Compare December 26, 2019 21:10

rust-highfive assigned Amanieu and unassigned RalfJung Dec 30, 2019

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2020

foeb added 2 commits January 16, 2020 18:26

Document unsafe blocks in core::{cell, str, sync}

19fdc6e

Fix typo "gurantees -> guarantees"

a93e99c

foeb and others added 3 commits January 16, 2020 18:27

Apply suggestions from code review

e0140ff

Co-Authored-By: Ralf Jung <post@ralfj.de>

Elaborate on SAFETY comments

ca2fae8

Move comments for tidy

c103c28

foeb force-pushed the 66219-document-unsafe-sync-cell-str branch from 9f4f19a to c103c28 Compare January 17, 2020 02:39

Fix formatting: ./x.py fmt

0f2ee49

Add SAFETY comment for atomic example

022a7de

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2020

Dylan-DPC-zz mentioned this pull request Jan 17, 2020

Rollup of 5 pull requests #68310

Closed

Dylan-DPC-zz mentioned this pull request Jan 17, 2020

Rollup of 4 pull requests #68311

Merged

bors merged commit 022a7de into rust-lang:master Jan 17, 2020

RalfJung reviewed Sep 10, 2020

View reviewed changes

ChayimFriedman2 mentioned this pull request Apr 7, 2022

Correct safety reasoning in str::make_ascii_{lower,upper}case() #95753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document unsafe blocks in core::{cell, str, sync} #66564

Document unsafe blocks in core::{cell, str, sync} #66564

foeb commented Nov 20, 2019

rust-highfive commented Nov 20, 2019

RalfJung commented Nov 20, 2019

RalfJung commented Nov 20, 2019

JohnCSimon commented Nov 24, 2019

JohnCSimon commented Nov 30, 2019

foeb commented Nov 30, 2019

JohnCSimon commented Dec 14, 2019

RalfJung commented Dec 14, 2019

RalfJung Dec 22, 2019

RalfJung Dec 22, 2019

RalfJung left a comment •

edited

Loading

RalfJung Dec 22, 2019

foeb Dec 26, 2019

Amanieu Dec 31, 2019

bors commented Dec 23, 2019

RalfJung commented Dec 30, 2019

Amanieu commented Dec 31, 2019

Dylan-DPC-zz commented Jan 16, 2020

rust-highfive commented Jan 17, 2020

foeb commented Jan 17, 2020

Dylan-DPC-zz commented Jan 17, 2020

bors commented Jan 17, 2020

bors commented Jan 17, 2020

Dylan-DPC-zz commented Jan 17, 2020

bors commented Jan 17, 2020

RalfJung Sep 10, 2020 •

edited

Loading

Document unsafe blocks in core::{cell, str, sync} #66564

Document unsafe blocks in core::{cell, str, sync} #66564

Conversation

foeb commented Nov 20, 2019

rust-highfive commented Nov 20, 2019

RalfJung commented Nov 20, 2019

RalfJung commented Nov 20, 2019

JohnCSimon commented Nov 24, 2019

JohnCSimon commented Nov 30, 2019

foeb commented Nov 30, 2019

JohnCSimon commented Dec 14, 2019

RalfJung commented Dec 14, 2019

RalfJung Dec 22, 2019

Choose a reason for hiding this comment

RalfJung Dec 22, 2019

Choose a reason for hiding this comment

RalfJung left a comment • edited Loading

Choose a reason for hiding this comment

RalfJung Dec 22, 2019

Choose a reason for hiding this comment

foeb Dec 26, 2019

Choose a reason for hiding this comment

Amanieu Dec 31, 2019

Choose a reason for hiding this comment

bors commented Dec 23, 2019

RalfJung commented Dec 30, 2019

Amanieu commented Dec 31, 2019

Dylan-DPC-zz commented Jan 16, 2020

rust-highfive commented Jan 17, 2020

foeb commented Jan 17, 2020

Dylan-DPC-zz commented Jan 17, 2020

bors commented Jan 17, 2020

bors commented Jan 17, 2020

Dylan-DPC-zz commented Jan 17, 2020

bors commented Jan 17, 2020

RalfJung Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung left a comment •

edited

Loading

RalfJung Sep 10, 2020 •

edited

Loading