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 UB caused by casting a Rust fn ptr to a *c_void (BMO bug #1351497) #81

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

kinetiknz
Copy link
Collaborator

Use Option<extern fn()> to represent potentially-null function pointer.

This addresses https://bugzilla.mozilla.org/show_bug.cgi?id=1351497, which was filed as a rustc bug in rust-lang/rust#40913, but revealed to be undefined behaviour that happened to work with earlier compiler versions.

As the comment removed in this patch describes, the only reason this code existed was to work around a limitation of rusty-cheddar.

Not sure what to do about the rusty-cheddar issue. We can patch the generated header manually (by replacing the bogus mp4parse_io member Option read with intptr_t (*read)(uint8_t* buffer, uintptr_t size, void* userdata) for now, but that's not an ideal solution.

@kinetiknz kinetiknz requested a review from rillian March 30, 2017 05:17
@rillian
Copy link
Contributor

rillian commented Mar 30, 2017

I don't want to go back to maintaining mp4parse.h by hand, so I think we need to fix cheddar if we're going to use Option<extern fn ...>. Or move from cheddar to bindgen. :/

This addresses https://bugzilla.mozilla.org/show_bug.cgi?id=1351497, which
was filed as a rustc bug in rust-lang/rust#40913,
but revealed to be undefined behaviour that happened to work with earlier
compiler versions.

As the comment removed in this patch describes, the only reason this code
existed was to work around a limitation of rusty-cheddar.
@kinetiknz
Copy link
Collaborator Author

Okay, so I've forked rusty-cheddar and fixed it: kinetiknz/moz-cheddar@bb819e5

And repushed this fix with the rusty-cheddar dep switched to my repo.

@rillian
Copy link
Contributor

rillian commented Mar 31, 2017

Most awesome, thanks!

@rillian rillian merged commit 2f595d9 into mozilla:master Mar 31, 2017
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.

2 participants