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

Projects
None yet
2 participants
@schwehr
Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

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

Catch over range lam0 in proj_etmerc.c
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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

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

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