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 CFRunLoop types to avoid UB #349

Merged
merged 6 commits into from Nov 12, 2019
Merged

Fix CFRunLoop types to avoid UB #349

merged 6 commits into from Nov 12, 2019

Conversation

@wez
Copy link
Contributor

wez commented Nov 12, 2019

A number of callback functions associated with the CFRunLoop types are
allowed to be NULL to evoke default behavior.

Prior to this commit the definition of those structs prevented passing
in null which meant resorting to transmute or other tricks to
effectively set null, but with recent compiler versions the compiler
emits ud2 instructions and triggers a fault at runtime.

The correct resolution for this is to define these fields as Optional
callbacks and that is what this commit does.

I've used this approach successfully here in another project:
Refs: wez/wezterm@398f333

I've bumped up the version for the crate as part of this commit
because it effectively changes the API around these structs.

wez added 3 commits Nov 12, 2019
A number of callback functions associated with the CFRunLoop types are
allowed to be NULL to evoke default behavior.

Prior to this commit the definition of those structs prevented passing
in null which meant resorting to transmute or other tricks to
effectively set null, but with recent compiler versions the compiler
emits `ud2` instructions and triggers a fault at runtime.

The correct resolution for this is to define these fields as `Option`al
callbacks and that is what this commit does.

I've used this approach successfully here in another project:
Refs: wez/wezterm@398f333

I've bumped up the version for the crate as part of this commit
because it effectively changes the API around these structs.
@lqf96
Copy link
Contributor

lqf96 commented Nov 12, 2019

Since the definition of CFRunLoopTimerContext has changed, I think the test in core-foundation/src/runloop.rs should also be updated... It's a simple update though, just replace mem::zeroed() with None...

Copy link
Member

jdm left a comment

Thanks for fixing this!

core-foundation-sys/src/runloop.rs Show resolved Hide resolved
core-foundation-sys/src/runloop.rs Show resolved Hide resolved
io-surface/Cargo.toml Show resolved Hide resolved
core-text/Cargo.toml Show resolved Hide resolved
core-graphics/Cargo.toml Show resolved Hide resolved
cocoa/Cargo.toml Show resolved Hide resolved
wez added 2 commits Nov 12, 2019
@jdm
Copy link
Member

jdm commented Nov 12, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2019

📌 Commit a119788 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2019

Testing commit a119788 with merge 0a3ac83...

bors-servo added a commit that referenced this pull request Nov 12, 2019
Fix CFRunLoop types to avoid UB

A number of callback functions associated with the CFRunLoop types are
allowed to be NULL to evoke default behavior.

Prior to this commit the definition of those structs prevented passing
in null which meant resorting to transmute or other tricks to
effectively set null, but with recent compiler versions the compiler
emits `ud2` instructions and triggers a fault at runtime.

The correct resolution for this is to define these fields as `Option`al
callbacks and that is what this commit does.

I've used this approach successfully here in another project:
Refs: wez/wezterm@398f333

I've bumped up the version for the crate as part of this commit
because it effectively changes the API around these structs.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2019

☀️ Test successful - checks-travis
Approved by: jdm
Pushing 0a3ac83 to master...

@bors-servo bors-servo merged commit a119788 into servo:master Nov 12, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@jdm jdm mentioned this pull request Nov 12, 2019
@wez wez deleted the wez:timertypes branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.