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

Crash while serializing data into JSON. #238

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Crash while serializing data into JSON. #238

merged 1 commit into from
Nov 29, 2022

Conversation

matux
Copy link
Collaborator

@matux matux commented Nov 29, 2022

Description of the change

This PR adds a low-level layer of protection to the serialization component of Rollbar to avoid a rare crash where the received raw input to be serialized is not an NSObject, but either a primitive or an unserializable Swift type (eg. a tuple).

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@matux matux requested a review from ijsnow November 29, 2022 12:14
@matux matux self-assigned this Nov 29, 2022
@matux matux requested a review from cyrusradfar November 29, 2022 14:24
Copy link

@ijsnow ijsnow left a comment

Choose a reason for hiding this comment

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

A little more objective c-y than my digging beneath the surface of react native libraries showed me but the logic tracks with me. Just a couple comments for you to double check before merging.

@matux matux merged commit 66f620f into release Nov 29, 2022
@matux matux deleted the null_crash branch November 29, 2022 17:37
@matux
Copy link
Collaborator Author

matux commented Nov 29, 2022

A little more objective c-y than my digging beneath the surface of react native libraries showed me but the logic tracks with me. Just a couple comments for you to double check before merging.

Just to annotate:

#define IS_NSOBJECT(_X_) (strchr("@#", @encode(typeof(_X_))[0]) != NULL)

This is some serious dark voodoo magic which leverages Apple's type encoding to check whether a memory address is in fact an Objective-C type, and not a primitive, or a Swift type or some other stuff we can't poke in our code.

Truth is, the fact that this kind of stuff is necessary, should be reason enough for Apple to create a completely new programming langu—Oh, wait.

@ijsnow
Copy link

ijsnow commented Nov 29, 2022

You'd think they'd be in some sort of hurry to create their new language and would do so in a... can't think of the right word... quick(?) manner?

Cool, thanks for the annotation! Now I know about type encodings

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