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

ref: convert an sprintf call to snprintf #2866

Merged
merged 17 commits into from
May 18, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Apr 6, 2023

I tried running some tests with the address sanitizer and compilation failed because of these usages of sprintf. stdio.h now declares it with the following deprecation notice:

This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.

I'm not sure why that only surfaces when running with ASAN.

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1229.73 ms 1249.17 ms 19.43 ms
Size 20.76 KiB 435.22 KiB 414.46 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dc0db9e 1222.10 ms 1240.90 ms 18.80 ms
d3abae0 1200.36 ms 1224.22 ms 23.87 ms
8f397a7 1236.76 ms 1256.76 ms 20.00 ms
ff09c7e 1244.86 ms 1246.68 ms 1.82 ms
17afc4b 1228.94 ms 1251.10 ms 22.16 ms
bd05478 1233.82 ms 1255.56 ms 21.74 ms
bd2afa6 1245.24 ms 1263.18 ms 17.94 ms
2719ce6 1211.75 ms 1237.16 ms 25.41 ms
7bc3c0d 1261.16 ms 1278.38 ms 17.22 ms
28333b6 1247.29 ms 1262.51 ms 15.22 ms

App size

Revision Plain With Sentry Diff
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
d3abae0 20.76 KiB 434.92 KiB 414.16 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
ff09c7e 20.76 KiB 427.77 KiB 407.00 KiB
17afc4b 20.76 KiB 436.25 KiB 415.49 KiB
bd05478 20.76 KiB 432.33 KiB 411.57 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
2719ce6 20.76 KiB 435.13 KiB 414.37 KiB
7bc3c0d 20.76 KiB 427.36 KiB 406.59 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB

Previous results on branch: armcknight/fix/deprecated-sprintf-usages

Startup times

Revision Plain With Sentry Diff
5c2a153 1202.60 ms 1230.16 ms 27.56 ms
4773882 1197.64 ms 1229.50 ms 31.86 ms
d435944 1263.14 ms 1266.49 ms 3.35 ms
98e4852 1225.51 ms 1254.26 ms 28.75 ms
5a4e93a 1243.60 ms 1257.86 ms 14.26 ms
dabc1ae 1249.12 ms 1263.98 ms 14.86 ms
4a138bd 1215.98 ms 1235.84 ms 19.86 ms

App size

Revision Plain With Sentry Diff
5c2a153 20.76 KiB 433.21 KiB 412.45 KiB
4773882 20.76 KiB 435.23 KiB 414.47 KiB
d435944 20.76 KiB 433.22 KiB 412.46 KiB
98e4852 20.76 KiB 432.17 KiB 411.41 KiB
5a4e93a 20.76 KiB 433.22 KiB 412.46 KiB
dabc1ae 20.76 KiB 431.97 KiB 411.21 KiB
4a138bd 20.76 KiB 432.17 KiB 411.41 KiB

@armcknight

This comment was marked as outdated.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring, @armcknight. Let's please add a test ensuring we don't crash in SentryCrashReport.

Sources/SentryCrash/Recording/SentryCrashReport.c Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM.
Just one suggestion

Sources/SentryCrash/Recording/Tools/SentryCrashJSONCodec.c Outdated Show resolved Hide resolved
@armcknight armcknight closed this Apr 25, 2023
@philipphofmann
Copy link
Member

@armcknight, can you share some info on why you closed this PR?

@armcknight
Copy link
Member Author

@philipphofmann I'm not really sure, I must have hit the button by accident.

@armcknight armcknight reopened this May 17, 2023
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #2866 (6eda779) into main (fcde045) will decrease coverage by 0.065%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #2866       +/-   ##
=============================================
- Coverage   88.857%   88.792%   -0.065%     
=============================================
  Files          492       492               
  Lines        52967     52983       +16     
  Branches     18971     18969        -2     
=============================================
- Hits         47065     47045       -20     
- Misses        4948      4977       +29     
- Partials       954       961        +7     
Impacted Files Coverage Δ
...SentryCrash/Recording/Tools/SentryCrashJSONCodec.c 83.078% <100.000%> (ø)
...ntryTests/SentryCrash/SentryCrashJSONCodec_Tests.m 99.854% <100.000%> (+0.001%) ⬆️

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcde045...6eda779. Read the comment docs.

@philipphofmann
Copy link
Member

So what's stopping us from merging it @armcknight 😃?

@armcknight
Copy link
Member Author

Nothing, tests pass so should be g2g!

@armcknight armcknight changed the title ref: replace deprecated/unsafe sprintf calls with equivalent snprintf calls ref: convert an sprintf call to snprintf May 18, 2023
@armcknight armcknight merged commit 736495a into main May 18, 2023
@armcknight armcknight deleted the armcknight/fix/deprecated-sprintf-usages branch May 18, 2023 11:50
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- convert an sprintf call to snprintf ([#2866](https://github.com/getsentry/sentry-cocoa/pull/2866))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 6eda779

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