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 undefined behavior in regex_macros #201

Merged
merged 1 commit into from
Apr 15, 2016

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Apr 10, 2016

The value read in contains may be uninitialized, and using such a value is undefined behavior. In particular with LLVM, where s is undef and can return a different value each time it is read. The solution is to use a volatile load to ensure we already get a real value, not an undef.

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2016

Note that this is currently blocked on rust-lang/rust#32804

@BurntSushi
Copy link
Member

@Amanieu Hmm, are you sure? The whole point of the sparse set optimization is that it never actually reads from uninitialized memory. See: http://research.swtch.com/sparse

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2016

Actually it does read from uninitialized memory (sparse[i]):

If i is not in the set, then it doesn't matter what sparse[i] is set to: either sparse[i] will be bigger than n or it will point at a value in dense that doesn't point back at it.

@BurntSushi
Copy link
Member

@Amanieu ah, I see, yeah you're right. (I'd also be fine removing the optimization altogether.)

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2016

Removing the optimization entirely is also an option, I think either way is fine.

@alexcrichton
Copy link
Member

From an LLVM point of view, I believe it's correct that it's not undefined behavior to read undefined memory, it just returns undef. I also think that it's not undefined behavior to use that undef value, it just propagates. For example if you add an undef value to something else, the returned value is always undef.

The unefined behavior, however, is when you start doing things like branching and such. For example here if s can be undefined then the comparison < can be optimized away (as can the bounds checks in the array access), which then will cause undefined behavior.

(nice catch regardless!)

@Amanieu
Copy link
Member Author

Amanieu commented Apr 15, 2016

This should work now that read_volatile has been stabilized in the latest nightly.

@BurntSushi
Copy link
Member

Hmm, just waiting on the Windows CI to go through.

@BurntSushi BurntSushi merged commit bf6bc5f into rust-lang:master Apr 15, 2016
@BurntSushi
Copy link
Member

Awesome, thank you! :-)

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.

3 participants