Skip to content

Fix undefined behavior in Rc/Arc allocation - #54922

Merged
bors merged 1 commit into
rust-lang:masterfrom
murarth:rc-ub-fix
Nov 6, 2018
Merged

Fix undefined behavior in Rc/Arc allocation#54922
bors merged 1 commit into
rust-lang:masterfrom
murarth:rc-ub-fix

Conversation

@murarth

@murarth murarth commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

Manually calculate allocation layout for Rc/Arc to avoid undefined behavior

Closes #54908

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2018
@Centril

Centril commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned sfackler Oct 9, 2018
@RalfJung

RalfJung commented Oct 9, 2018

Copy link
Copy Markdown
Member

Could you add a comment saying why we do it this way and not the old way?

Other than that, I agree the alignment problem is gone. However, I do not know nearly enough about layouts to say if that's a correct way to compute these things. Assigning to someone who knows more (feel free to pick someone else).

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned RalfJung Oct 9, 2018
@RalfJung RalfJung mentioned this pull request Oct 10, 2018
Closed
@gnzlbg

gnzlbg commented Oct 10, 2018

Copy link
Copy Markdown
Contributor

cc @SimonSapin @Amanieu

@nikomatsakis nikomatsakis mentioned this pull request Oct 11, 2018
Closed
@TimNN

TimNN commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

Ping from triage! This PR requires your review @eddyb. Also this PR was references from rust-lang/unsafe-code-guidelines#35, is it blocked on that issue?

@eddyb

eddyb commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

This is not a compiler layout, but I suspect it's correct.

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Nov 5, 2018
RalfJung
RalfJung reviewed Nov 5, 2018
View reviewed changes
Comment thread src/liballoc/rc.rs Outdated
@alexcrichton

alexcrichton commented Nov 5, 2018

Copy link
Copy Markdown
Member

Thanks for the PR and sorry for the delay! This looks good to go to me with @RalfJung's comment

@alexcrichton

alexcrichton commented Nov 5, 2018

Copy link
Copy Markdown
Member

If possible, it'd also be great to have a regression test for this!

@RalfJung

RalfJung commented Nov 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

If possible, it'd also be great to have a regression test for this!

We'll have one in miri. I have no idea how to make one with rustc only.

@murarth
Fix undefined behavior in Rc/Arc allocation
d60290f 
Manually calculate allocation layout for `Rc`/`Arc` to avoid undefined behavior
@murarth

murarth commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

I've added a more detailed comment and the tests have passed.

@alexcrichton

alexcrichton commented Nov 5, 2018

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented Nov 5, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit d60290f has been approved by alexcrichton

@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 Nov 5, 2018
@bors

bors commented Nov 5, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit d60290f with merge 65e485d...

bors added a commit that referenced this pull request Nov 5, 2018
@bors
Auto merge of #54922 - murarth:rc-ub-fix, r=alexcrichton
65e485d 
Fix undefined behavior in Rc/Arc allocation

Manually calculate allocation layout for `Rc`/`Arc` to avoid undefined behavior

Closes #54908
@bors

bors commented Nov 6, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 65e485d to master...

@bors
bors merged commit d60290f into rust-lang:master Nov 6, 2018
@murarth
murarth deleted the rc-ub-fix branch November 6, 2018 02:23
@RalfJung

RalfJung commented Nov 7, 2018

Copy link
Copy Markdown
Member

Hm, this RFC made miri fail even if validation is turned off. Still investigating.

@RalfJung

RalfJung commented Nov 7, 2018

Copy link
Copy Markdown
Member

The error is that an allocation has size 32 but gets deallocated with size 28.

I assume the layout computations done here are incorrect.

@RalfJung RalfJung mentioned this pull request Nov 7, 2018
Closed
@RalfJung

RalfJung commented Nov 7, 2018

Copy link
Copy Markdown
Member

Reported as #55747.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@alexcrichton alexcrichton

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@murarth @rust-highfive @Centril @RalfJung @gnzlbg @TimNN @eddyb @alexcrichton @bors @sfackler