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

ICU-22251 Move sprintf to snprintf. #2291

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

FrankYFTang
Copy link
Contributor

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22251
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

richgillam
richgillam previously approved these changes Jan 23, 2023
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

This all looks good. Looks mechanical, except for the calculation of the buffer size. I didn't review all of those carefully, but the ones I did all looked right.

richgillam
richgillam previously approved these changes Jan 24, 2023
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

New stuff looks good too.

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Jan 24, 2023
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Member

@roubert roubert left a comment

Choose a reason for hiding this comment

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

I really don't think that you should be hardcoding any of these numbers, that wouldn't get us any real improved memory safety, it would just move the places where things could go wrong.

Instead, existing code like this:

char tmp[16];
sprintf(tmp, "%d", value);

Should be replaced with new code like this:

char tmp[16];
snprintf(tmp, sizeof tmp, "%d", value);

Or, when that's not possible, some kind of logic / argument passing that ensures that the value used is ultimately calculated by the compiler from the actual buffer size.

@FrankYFTang FrankYFTang reopened this Jan 25, 2023
@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Jan 25, 2023

Should be replaced with new code like this:

char tmp[16];
snprintf(tmp, sizeof tmp, "%d", value);

DONE

richgillam
richgillam previously approved these changes Jan 25, 2023
Copy link
Contributor

@richgillam richgillam 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, and I think Fredrik's recommendation is a good one and makes the code more readable and bulletproof. But I think you missed a couple spots...

@@ -1157,7 +1157,7 @@ static void TestICUDataName()
}

/* Only major number is needed. */
sprintf(expectDataName, "%s%d%c",
snprintf(expectDataName, 20, "%s%d%c",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you didn't use sizeof here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somehow I missed this

icu4c/source/test/cintltst/udatatst.c Show resolved Hide resolved
icu4c/source/test/intltest/alphaindextst.cpp Outdated Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

PTAL

Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

It all looks fine. Cut and print.

icu4c/source/test/cintltst/udatatst.c Show resolved Hide resolved
@FrankYFTang
Copy link
Contributor Author

I really don't think that you should be hardcoding any of these numbers, that wouldn't get us any real improved memory safety, it would just move the places where things could go wrong.

Instead, existing code like this:

char tmp[16];
sprintf(tmp, "%d", value);

Should be replaced with new code like this:

char tmp[16];
snprintf(tmp, sizeof tmp, "%d", value);

Or, when that's not possible, some kind of logic / argument passing that ensures that the value used is ultimately calculated by the compiler from the actual buffer size.

done

@FrankYFTang FrankYFTang removed the request for review from roubert January 26, 2023 07:22
@FrankYFTang FrankYFTang dismissed roubert’s stale review January 26, 2023 07:23

cannot find the item

@FrankYFTang FrankYFTang merged commit de0a286 into unicode-org:main Jan 26, 2023
@FrankYFTang FrankYFTang deleted the ICU-22251-sprintf branch January 26, 2023 07:23
Manishearth pushed a commit to Manishearth/icu that referenced this pull request Feb 10, 2023
sffc pushed a commit that referenced this pull request Feb 10, 2023
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