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

Catch over range lam0 in proj_etmerc.c #899

Merged
merged 1 commit into from Mar 25, 2018

Conversation

@schwehr
Copy link
Contributor

@schwehr schwehr commented Mar 25, 2018

lam0 of inf caused a nan in the cast to int.
Picked +/-1000 for lam0 as a guess for what constitutes reduculously large values.

proj_etmerc.c:361:16: runtime error: nan is outside the range of representable values of type 'int'

Found with autofuzz: UndefinedBehaviorSanitizer: float-cast-overflow

@schwehr
Copy link
Contributor Author

@schwehr schwehr commented Mar 25, 2018

poc for gdal/autotest2/cpp/ogr:ogr_srs_proj4_fuzzer:

+proj=pipeline	++proj=utm +lon_0=7e902
+stex	+step	+ste
@kbevers
Copy link
Member

@kbevers kbevers commented Mar 25, 2018

The fix seems find. Thanks for catching the problem. May I ask though that you move the P->lam0 check up ~15 lines, below the P->es check, so that the error checking is done before entering the main logic of the code? We[0] try to follow that principle in new code. The reason being that you might as well fail as early as possible and save some CPU cycles and that it removes unnecessary mental obstructions when reading the code since the algorithm is written in it's pure form without any detours.

[0] at least @busstoptaktik and me

lam0 of inf caused a nan in the cast to int.
Picked +/-1000 for lam0 as a guess for what constitutes reduculously large values.

proj_etmerc.c:361:16: runtime error: nan is outside the range of representable values of type 'int'

Found with autofuzz: UndefinedBehaviorSanitizer: float-cast-overflow
@schwehr schwehr force-pushed the schwehr:proj_etmerc-inf-zone-b76304619 branch from 55bba97 to e52ab92 Mar 25, 2018
@schwehr
Copy link
Contributor Author

@schwehr schwehr commented Mar 25, 2018

I'm of the same mindset... range check up front as much as possible.

Hopefully this redo of the patch doesn't have any surprises. I don't know proj well enough to have a sense of when something might have a surprise side effect.

@kbevers
Copy link
Member

@kbevers kbevers commented Mar 25, 2018

This should be fine. Generally speaking, when inside a PJ_*.c file you will only be able to cause side effects in code within that file.

@kbevers kbevers merged commit 970bad5 into OSGeo:master Mar 25, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 75.757%
Details
@kbevers kbevers added this to the 5.1.0 milestone Mar 25, 2018
@schwehr schwehr deleted the schwehr:proj_etmerc-inf-zone-b76304619 branch Mar 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants