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

Check for errors from str_path_to_cfstring_ref. #302

Merged
merged 1 commit into from
May 4, 2021

Conversation

jorendorff
Copy link
Contributor

Closes #301.

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this fixes it or rather just works in case the race condition for our runtime panic isn't triggered ?

@@ -281,6 +281,10 @@ impl FsEventWatcher {
unsafe {
let mut err: cf::CFErrorRef = ptr::null_mut();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably investigate whether this should be handled instead of ignored (and deallocated)

@0xpr03 0xpr03 merged commit 86421e5 into notify-rs:main May 4, 2021
@jorendorff jorendorff deleted the jorendorff/null-check branch May 4, 2021 12:16
@jorendorff
Copy link
Contributor Author

Are you sure this fixes it or rather just works in case the race condition for our runtime panic isn't triggered ?

Medium-high confidence that this fixes the actual problem. Here's the evidence:

  • Before the change, the test crashed pretty reliably for me, say 95-98% of test runs, except in lldb. The first thing I tried is just adding assert!(!cf_path.is_null()) there, and it stopped the crashes immediately; they became regular Rust panics.

  • While I was reducing this from my real application, I ended up making 8 versions of the test (each containing fewer and fewer random bits of my app) and the change, backported to 4.0, makes them all pass.

To get a bit more confidence, one could write a program that passes a null second argument to cf::CFArrayAppendValue, to see if it crashes with this strange message about a "foreign exception".

0xpr03 added a commit that referenced this pull request May 4, 2021
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 4, 2021
(cherry picked from commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 4, 2021
(cherry picked from commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 6, 2021
(cherry picked from commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 6, 2021
(cherry picked from commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 6, 2021
(based on commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 7, 2021
(based on commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
0xpr03 added a commit that referenced this pull request May 8, 2021
(based on commit 86421e5)
Signed-off-by: Aron Heinecke <[email protected]>
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.

"fatal runtime error: Rust cannot catch foreign exceptions" with remove_dir on Mac
2 participants