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

Replace sprintf in JSON encoder #1034

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Replace sprintf in JSON encoder #1034

merged 1 commit into from
Mar 16, 2021

Conversation

kstenerud
Copy link
Contributor

@kstenerud kstenerud commented Mar 15, 2021

Goal

sprintf can call malloc, which can cause lockups in a crash scenario. Replace the sprintf calls in the JSON encoder with async-safe code. The new code is a simplified algorithm that always outputs in exponential form when the exponent is not 0. The result is the same effective value as before when ingested by a machine, but it may look a little strange to a human.

Testing

  • Added more unit tests specifically targeting the integer and float encoding in the JSON encoder.
  • Re-ran unit and e2e tests.

@github-actions
Copy link

Infer: No issues found 🎉

OCLint: No issues found 🎉

Bugsnag.framework binary size increased by 640 bytes from 1,084,896 to 1,085,536

Generated by 🚫 Danger

@kstenerud kstenerud force-pushed the 6161-sprintf-replacement branch 2 times, most recently from bd6c8bf to f422b59 Compare March 15, 2021 14:11
@nickdowell
Copy link
Contributor

What effect does this have on the generated output? (if any)

There does not appear to be much test coverage of the floating point output - is it worth adding some test cases for the various possible inputs?

@kstenerud
Copy link
Contributor Author

kstenerud commented Mar 15, 2021

I'm still working on the tests. This was supposed to be a draft. Github is annoying in that way because any change to the PR details you make such as changing the base branch will automatically reset the PR type to "release".

@kstenerud kstenerud force-pushed the 6161-sprintf-replacement branch 4 times, most recently from 282fc14 to 767924c Compare March 16, 2021 09:28
@kstenerud kstenerud force-pushed the 6161-sprintf-replacement branch from 767924c to 8af36fc Compare March 16, 2021 10:40
Copy link
Contributor

@nickdowell nickdowell left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Successfully ran features/crashprobe.feature:159 20 times on my Mac without a failure, whereas the next branch failed on the 5th attempt.

@kstenerud kstenerud merged commit c9492d2 into next Mar 16, 2021
@kstenerud kstenerud deleted the 6161-sprintf-replacement branch March 16, 2021 13:59
nickdowell added a commit that referenced this pull request Mar 18, 2021
@nickdowell nickdowell mentioned this pull request Mar 24, 2021
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.

3 participants