Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

When generating an NX crash, previously the code would explicitly jump... #6

Merged
2 commits merged into from
May 12, 2017

Conversation

gwynne
Copy link

@gwynne gwynne commented Apr 14, 2017

to NULL, which modern versions of Clang correctly optimize out as provable undefined behavior (the compiler is free to do whatever it wants if it can prove that the code will always dereference NULL). Instead, map a valid memory space without the execute permission and jump to that pointer.

…p to NULL, which modern versions of Clang correctly optimize out as provable undefined behavior (the compiler is free to do whatever it wants if it can prove that the code will always dereference NULL). Instead, map a valid memory space without the execute permission and jump to that pointer.
@msftclas
Copy link

@gwynne,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@ElektrojungeAtWork
Copy link

I just "stole" this for the Mobile Center SDK =)

1) Because the jump to a bad address prevents the last “correct” frame from showing up, act more like real code and call another function that definitively isn’t inlined.
2) munmap() the pointer after the crash for good form.
@ghost
Copy link

ghost commented Apr 21, 2017

Since Xcode 8.3 this test case didn't cause a crash any longer due to the reasons @gwynne explained. But the new test code will return different results, e.g. it is more likely now to get the proper filename and line numbers. Here is what HockeyApp would (correctly) return, and therefor the result not being incomplete but complete:

0 ???           0x0000000100fc0000 0x0 + 0
1 CrashLibiOS   0x00000001000efdd0 real_NXcrash (CRLCrashNXPage.m:41)
2 CrashLibiOS   0x00000001000efd88 -[CRLCrashNXPage crash] (CRLCrashNXPage.m:49)
3 CrashProbeiOS 0x000000010002a84c -[CRLDetailViewController doCrash] (CRLDetailViewController.m:53)

To publish this test change properly, it would only be right that every contributing service could update their test results and we'll publish all changes (ideally) in one update. What do you think?

Calling out the previous contributors on https://github.com/bitstadium/CrashProbe-Data: @QuantumNightmare @kattrali @finik @HazAT

@finik
Copy link

finik commented Apr 21, 2017

Makes sense, we are getting similar results. Added a commit with an update to our pull request.

@HazAT
Copy link

HazAT commented Apr 24, 2017

We are in for that 👍

Since everybody has to redo their tests now, can we also discuss the result of the Swift test (probably in another issue)?

Referencing the previous discussion between @anlinde @mitsuhiko bitstadium/CrashProbe-Data#15 (comment)
https://github.com/bitstadium/CrashProbe-Data/blob/master/ios/22/_sentry_arm64.crash#L12

Just wanted to get the opinion of other contributors about the issue, since we still think our result is not incomplete it shows more information. The result that we propose is matching what lldb/xcode and other systems do (eg: show inlined frame information).

-> #7

@ghost
Copy link

ghost commented Apr 24, 2017

@HazAT Please open an extra ticket for that discussion. Thanks :)

@HazAT HazAT mentioned this pull request Apr 24, 2017
@ghost ghost merged commit 3c4f796 into master May 12, 2017
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants