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

Remove remaining sprintf() uses, and deprecate proj_rtodms() by newly added proj_rtodms2() #3431

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 3, 2022

Fixes #3423

@rouault rouault added this to the 9.2.0 milestone Nov 3, 2022
Co-authored-by: Mike Taves <mwtoews@gmail.com>
@rouault rouault merged commit 5b9303d into OSGeo:master Nov 7, 2022
@seanm
Copy link
Contributor

seanm commented Nov 7, 2022

github didn't notify me about this PR until just now, when you closed it, so I'm only reviewing the code now, but I think there is a mistake. You did not account for the pointer arithmetic on ss that I mentioned in #3423.

char *ss = s;
...
if (!pos) { *ss++ = '-'; sign = 0; }

After this point, s and ss are not always pointing to the same place.

Then you do:

snprintf(ss,sizeof_s,...

What you need is a sizeof_ss that starts the same as sizeof_s but gets decremented when ss gets incremented.

Otherwise you have a buffer overflow. If I'm not mistaken...

rouault added a commit to rouault/PROJ that referenced this pull request Nov 7, 2022
@rouault
Copy link
Member Author

rouault commented Nov 7, 2022

You did not account for the pointer arithmetic on ss that I mentioned in #3423.

doh! thanks for the review. Should hopefully be fixed with #3441

rouault added a commit to rouault/PROJ that referenced this pull request Nov 7, 2022
rouault added a commit to rouault/PROJ that referenced this pull request Nov 7, 2022
rouault added a commit that referenced this pull request Nov 8, 2022
rtodms(): fix potential buffer overflow not dealt with with PR #3431 (master only)
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.

Replace remaining 4 calls to sprintf with snprintf
3 participants