Skip to content

from_u32_unchecked: check validity, and fix UB in Wtf8#72683

Merged
bors merged 5 commits intorust-lang:masterfrom
RalfJung:char-debug-check
May 31, 2020
Merged

from_u32_unchecked: check validity, and fix UB in Wtf8#72683
bors merged 5 commits intorust-lang:masterfrom
RalfJung:char-debug-check

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented May 28, 2020 •
edited
Loading

Fixes #72760

@rust-highfive
Copy link
Contributor

rust-highfive commented May 28, 2020

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2020
@RalfJung RalfJung force-pushed the char-debug-check branch from 6b4566d to 1956ada Compare May 28, 2020 08:52
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 28, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented May 28, 2020

📌 Commit 1956ada4c855673de327f16c91d11fdc4cd5a39c has been approved by Mark-Simulacrum

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2020
ollie27
ollie27 reviewed May 28, 2020
View reviewed changes
@RalfJung
Copy link
Member Author

RalfJung commented May 28, 2020

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 28, 2020
@RalfJung RalfJung force-pushed the char-debug-check branch from 1956ada to 52ed89a Compare May 28, 2020 12:04
@RalfJung RalfJung removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 29, 2020
@RalfJung
Copy link
Member Author

RalfJung commented May 29, 2020

@Mark-Simulacrum I fixed the endless recursion.

@RalfJung RalfJung added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2020
@RalfJung
Copy link
Member Author

RalfJung commented May 29, 2020

r? @Mark-Simulacrum
(sorry for double-ping)

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 29, 2020

@bors r+

No worries!

@bors
Copy link
Collaborator

bors commented May 29, 2020

📌 Commit 52ed89a has been approved by Mark-Simulacrum

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 29, 2020
@Dylan-DPC
Rollup merge of rust-lang#72683 - RalfJung:char-debug-check, r=Mark-S…
793967b 
…imulacrum

from_u32_unchecked: check validity when debug assertions are enabled
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 29, 2020
Closed
@RalfJung
Copy link
Member Author

RalfJung commented May 29, 2020

This actually caused CI to fail in #72736 (comment)

---- sys_common::wtf8::tests::wtf8_as_str stdout ----
thread 'sys_common::wtf8::tests::wtf8_as_str' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/char/convert.rs:102:33

---- sys_common::wtf8::tests::wtf8_code_points stdout ----
thread 'sys_common::wtf8::tests::wtf8_code_points' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/char/convert.rs:102:33

...

@bors r-

Our wtf8 code seems to have UB.^^

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2020
@RalfJung RalfJung changed the title from_u32_unchecked: check validity when debug assertions are enabled from_u32_unchecked: check validity, and fix UB in Wtf8 May 30, 2020
@RalfJung
Copy link
Member Author

RalfJung commented May 30, 2020

@SimonSapin I implemented the fix you suggested here.

@RalfJung RalfJung force-pushed the char-debug-check branch from dc8c1a0 to 9c627c3 Compare May 30, 2020 10:11
@RalfJung RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2020
Mark-Simulacrum
Mark-Simulacrum reviewed May 30, 2020
View reviewed changes
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 30, 2020

r=me with comment added, unless you want to wait for @SimonSapin's review in particular (but AFAICT this is just moving some code around so I feel comfortable approving)

@RalfJung
Copy link
Member Author

RalfJung commented May 30, 2020

I added comments but tried to avoid claiming that this is valid UTF-8... what do you think?

@RalfJung RalfJung force-pushed the char-debug-check branch from 05520e4 to 96615ce Compare May 30, 2020 15:17
RalfJung
RalfJung commented May 30, 2020
View reviewed changes
@RalfJung RalfJung force-pushed the char-debug-check branch from 96615ce to 0fb6e63 Compare May 30, 2020 15:27
Mark-Simulacrum
Mark-Simulacrum approved these changes May 30, 2020
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me

@RalfJung
Copy link
Member Author

RalfJung commented May 30, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented May 30, 2020

📌 Commit 0fb6e63 has been approved by Mark-Simulacrum

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2020
@RalfJung RalfJung mentioned this pull request May 30, 2020
Merged
@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 30, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request May 31, 2020
@RalfJung
Rollup merge of rust-lang#72683 - RalfJung:char-debug-check, r=Mark-S…
dd92106 
…imulacrum

from_u32_unchecked: check validity, and fix UB in Wtf8

Fixes rust-lang#72760
@RalfJung RalfJung mentioned this pull request May 31, 2020
Closed
@RalfJung RalfJung mentioned this pull request May 31, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2020
@bors
Auto merge of rust-lang#72813 - RalfJung:rollup-4ko6q8j, r=RalfJung
f6072ca 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#72683 (from_u32_unchecked: check validity, and fix UB in Wtf8)
 - rust-lang#72715 (Account for trailing comma when suggesting `where` clauses)
 - rust-lang#72745 (generalize Borrow<[T]> for Interned<'tcx, List<T>>)
 - rust-lang#72749 (Update stdarch submodule to latest head)
 - rust-lang#72781 (Use `LocalDefId` instead of `NodeId` in `resolve_str_path_error`)

Failed merges:

r? @ghost
@bors bors merged commit 3bbb475 into rust-lang:master May 31, 2020
bors added a commit to rust-lang/miri that referenced this pull request May 31, 2020
@bors
Auto merge of #1440 - RalfJung:wtf8, r=RalfJung
a8df047 
test WTF8 encoding corner cases

This adds a Miri-side test for rust-lang/rust#72760.

Blocked on rust-lang/rust#72683.
@RalfJung RalfJung deleted the char-debug-check branch June 1, 2020 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SimonSapin SimonSapin SimonSapin left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

+1 more reviewer

@ollie27 ollie27 ollie27 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OsString::from_wide (in Windows OsStringExt) is unsound

8 participants

@RalfJung @rust-highfive @Mark-Simulacrum @bors @SimonSapin @ollie27 @dtolnay @withoutboats