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 undefined behavior in Rc/Arc allocation #54922

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
10 participants
@murarth
Contributor

murarth commented Oct 9, 2018

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

Closes #54908

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Oct 9, 2018

r? @sfackler

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

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 9, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 9, 2018

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 referenced this pull request Oct 10, 2018

Closed

Representation of structs #11

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Oct 10, 2018

@TimNN

This comment has been minimized.

Contributor

TimNN commented Oct 23, 2018

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

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 5, 2018

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

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 5, 2018

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

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 5, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 5, 2018

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.

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

@murarth murarth force-pushed the murarth:rc-ub-fix branch from e251995 to d60290f Nov 5, 2018

@murarth

This comment has been minimized.

Contributor

murarth commented Nov 5, 2018

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

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 5, 2018

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

📌 Commit d60290f has been approved by alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

⌛️ Testing commit d60290f with merge 65e485d...

bors added a commit that referenced this pull request Nov 5, 2018

Auto merge of #54922 - murarth:rc-ub-fix, r=alexcrichton
Fix undefined behavior in Rc/Arc allocation

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

Closes #54908
@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

☀️ 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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@murarth murarth deleted the murarth:rc-ub-fix branch Nov 6, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 7, 2018

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 7, 2018

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

This comment has been minimized.

Member

RalfJung commented Nov 7, 2018

Reported as #55747.

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