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

[schwehr]

See #843

Found with autofuzz UndefinedBehaviorSanitizer (UBSan)

[schwehr]

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]

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.

[kbevers]

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.

[rouault]

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

[kbevers]

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.

[rouault]

Just

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

[schwehr]

Agreed. pj_is_nan would be less confusing.

[kbevers]

Let's go with that then.

@schwehr can you add that function to this PR?

[schwehr]

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.

[kbevers]

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]

@cffk are you happy with the changes?

[cffk]

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]

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

[kbevers]

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]

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

[cffk]

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]

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]

@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]

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

[kbevers]

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]

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

[kbevers]

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]

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]

Perfect. Thanks!

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