Skip to content

switch sprintf to snprintf#2175

Merged
jmklix merged 2 commits intomainfrom
sprintf_fix
Nov 8, 2022
Merged

switch sprintf to snprintf#2175
jmklix merged 2 commits intomainfrom
sprintf_fix

Conversation

@jmklix
Copy link
Copy Markdown
Member

@jmklix jmklix commented Nov 3, 2022

Issue #, if available:
Ran into this warning when building on mac:

"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."

Description of changes:
switch sprintf to snprintf

//from
sprintf(buf, "%lld", num);
//to
snprintf(buf, sizeof(buf), "%lld", num);

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment thread aws-cpp-sdk-core/source/external/cjson/cJSON.cpp Outdated
{
static char version[15];
sprintf(version, "%i.%i.%i", CJSON_AS4CPP_VERSION_MAJOR, CJSON_AS4CPP_VERSION_MINOR, CJSON_AS4CPP_VERSION_PATCH);
snprintf(version, sizeof(version), "%i.%i.%i", CJSON_AS4CPP_VERSION_MAJOR, CJSON_AS4CPP_VERSION_MINOR, CJSON_AS4CPP_VERSION_PATCH);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we sure we want to be laying changes on top of this copy/pasted CJSON implementation?

if we take an update, won't these changes just get wiped away?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since this is external code, it might be simpler to just disable the warnings from the top of the file, or in the CMakeLists.txt somehow ... something isolated with a high chance of being preserved if someone copy/pastes an updated cJSON.cpp into the repo

... discuss with the CPP team how they want to handle it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we are already C99, since we have already modified version of cJSON, we will unblock existing mac builds (i.e. I'm for merging this change).
And deal with external dependencies later.

@jmklix jmklix merged commit c8ac811 into main Nov 8, 2022
@jmklix jmklix deleted the sprintf_fix branch November 8, 2022 18:37
jmklix added a commit that referenced this pull request Aug 11, 2023
* switch sprintf to snprintf

* find correct sizeof output_pointer
amit-schreiber-firebolt pushed a commit to firebolt-analytics/aws-sdk-cpp that referenced this pull request May 8, 2025
* switch sprintf to snprintf

* find correct sizeof output_pointer
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