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

Handle nan float cast overflow in PJ_robin.c and nad_intr.c #887

Merged
merged 2 commits into from Mar 22, 2018

Conversation

Projects
None yet
4 participants
@schwehr
Contributor

schwehr commented Mar 21, 2018

See #843

Found with autofuzz UndefinedBehaviorSanitizer (UBSan)

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

I thought this was the preferred way to check for nan in C89.

t.lam == t.lam && ct->del.lam == ct->del.lam

But I get this from cppcheck:

src/nad_intr.c:14,style,duplicateExpression,Same expression on both sides of '=='.
@cffk

cffk requested changes Mar 22, 2018 edited

Change

indx.lam =
    t.lam == t.lam && ct->del.lam == ct->del.lam
    ? (int)floor(t.lam /= ct->del.lam)
    : 0;

to

t.lam /= ct->del.lam;
indx.lam = t.lam == t.lam ? (int)floor(t.lam) : 0;

Ditto for indx.phi. Then you get the intended side-effect of the
division and you also catch the catch where a NaN is generated with 0/0.

(void) P;

i = (int)floor((dphi = fabs(lp.phi)) * C1);
i = lp.phi == lp.phi ? (int)floor((dphi = fabs(lp.phi)) * C1) : -1;

This comment has been minimized.

@kbevers

kbevers Mar 22, 2018

Member

Is this pattern commonly used and recognized? I find it quite unintuitive and will definitely not understand this the next time I stumble on this part of the code.

Would it be possible to create a well-named function that takes care of this? Like pj_avoid_nan_on_assignment() (or something better...). Otherwise comments describing what's going on would be welcome.

This comment has been minimized.

@rouault

rouault Mar 22, 2018

Member

I would suggest adding an internal pj_is_nan() function to encapsulate NaN testing

This comment has been minimized.

@kbevers

kbevers Mar 22, 2018

Member

Works for me. I've got no idea how to write such a function, but I agree that it would be a better way to go in these situations.

This comment has been minimized.

@rouault

rouault Mar 22, 2018

Member

Just

int pj_is_nan(double x)
{
   /* cppcheck-suppress duplicateExpression */
   return x != x;
}

This comment has been minimized.

@schwehr

schwehr Mar 22, 2018

Contributor

Agreed. pj_is_nan would be less confusing.

This comment has been minimized.

@kbevers

kbevers Mar 22, 2018

Member

Let's go with that then.

@schwehr can you add that function to this PR?

This comment has been minimized.

@schwehr

schwehr Mar 22, 2018

Contributor

I gave it a go with the simplest possible implementation in pj_internal.c.

I avoided messing with isnan or other GCC custom craziness as seen here:

https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_port.h?rev=41149#L575

And I didn't patch other instances of x == x nan checks.

This comment has been minimized.

@kbevers

kbevers Mar 22, 2018

Member

Good call. We can add craziness when needed.

And I didn't patch other instances of x == x nan checks.

We can fix the rest as we come across them. Or if a master grepper want to search them out they can be done once and for all.

@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

@cffk are you happy with the changes?

@cffk

This comment has been minimized.

Contributor

cffk commented Mar 22, 2018

A couple of comments...

Non-urgent: The definition of HAVE_C99_MATH in configure.ac and
CMakeLists.txt should include a test for isnan. Then proj_internal.h
could include

#if (HAVE_C99_MATH)
#defined pj_is_nan isnan
#else
int pj_is_nan (double val);
#endif

and similiarly is proj_internal.c.

Urgent: surely the idiom

i = pj_is_nan(lp.phi) ? -1 : (int)floor((dphi = fabs(lp.phi)) * C1);

is dangerous? If lp.phi is a nan, the dphi doesn't get assigned. In
general, this will change the behavior of the code. Perhaps this is
OK, but you can't tell without further investigation.

BTW, surely assignments in arguments lists is a bad practice?

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

Let me do one more cleanup iteration. I agree that the assignment inside is not a great idea.

@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

Non-urgent: The definition of HAVE_C99_MATH in configure.ac and
CMakeLists.txt should include a test for isnan.

Good catch. That should be done, yes. If not done here a new issue should be added so we don't forget.

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

I have to check if fabs of NaN is safe or not.

@cffk

This comment has been minimized.

Contributor

cffk commented Mar 22, 2018

Surely the standard says fabs(+/-NAN) = +NAN. Generally floating point operations with nans are safe (the idea being that the floating point system is closed).

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

While I was pretty sure that fabs is just fine with NaN, it didn't hurt to check :) This passed UBSan in my testing env.

#include <cmath>
#include <limits>

#include "testing/base/public/gunit.h"

namespace {

TEST(FabsTest, HandlesNan) {
  constexpr double nan = std::numeric_limits<double>::quiet_NaN();
  constexpr double inf = std::numeric_limits<double>::infinity();

  EXPECT_TRUE(std::isnan(nan));
  EXPECT_TRUE(std::isnan(-nan));
  EXPECT_FALSE(std::isnan(inf));
  EXPECT_FALSE(std::isnan(-inf));

  EXPECT_TRUE(std::isnan(std::fabs(nan)));
  EXPECT_TRUE(std::isnan(std::fabs(-nan)));
  EXPECT_FALSE(std::isnan(std::fabs(inf)));
  EXPECT_FALSE(std::isnan(std::fabs(-inf)));
}

}  // namespace
@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

@schwehr Once you are happy with this, do you mind rebasing the commits into one for the addition of pj_is_nan and one for fixing the actual problem?

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

@kbevers Not having done that kind of rebase, can you point me at something that explains what you mean?

@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

Now that I look closer at your commits it might just be easier to reset all your commits (git reset --soft HEAD~6) and make two new commits. One for the pj_is_nan stuff and one for the bug fix. And then force push the new commits to overwrite what's already pushed to github.

Does that make sense?

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

That does make sense. I will give it a go.

@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

Awesome. Normally I would just squash everything when merging, but since this is actually two separate things I think it is best handled as two commits.

schwehr added some commits Mar 22, 2018

Handle nan float cast overflow in PJ_robin.c and nad_intr.c
Uses the new pj_is_nan to avoid x == x checks.
Removes an assignment from an arg list

@schwehr schwehr force-pushed the schwehr:float-cast-overflow branch from 3b8af6f to ec494f1 Mar 22, 2018

@schwehr

This comment has been minimized.

Contributor

schwehr commented Mar 22, 2018

There are now 2 commits in the history. The mac osx test timed out before. Hopefully this time around that travis-ci target will complete and that the timeout was just a flake.

@kbevers

This comment has been minimized.

Member

kbevers commented Mar 22, 2018

Perfect. Thanks!

The OS X test has been a bit flaky lately so don't worry about that.

@kbevers kbevers added this to the 5.1.0 milestone Mar 22, 2018

@kbevers kbevers merged commit 1160207 into OSGeo:master Mar 22, 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 increased (+0.008%) to 75.776%
Details

@schwehr schwehr deleted the schwehr:float-cast-overflow branch Mar 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment