mitab_mapheaderblock.cpp XScale and YScale in r38361

[Kurt Schwehr]

In reviewing r38361, Pete Klier commented 0.0 is not the only bad possibility for m_XScale and m_YScale.

If ReadDouble returns really close to zero (e.g. 1e-100), really large (e.g. -1.79769e+308 or 1.79769e+308), on of the NaNs or +/-inf, we don't want to be dividing by it, correct?

    m_XScale = ReadDouble();
    m_YScale = ReadDouble();
    if( m_XScale == 0.0 || m_YScale == 0.0 )
    {
        CPLError(CE_Failure, CPLE_FileIO,
              "ReadFromFile(): Null xscale and/or yscale");
        CPLFree(m_pabyBuf);
        m_pabyBuf = NULL;
        return -1;
    }

leads to

    if (m_nCoordOriginQuadrant==2 || m_nCoordOriginQuadrant==3 ||
        m_nCoordOriginQuadrant==0 )
        dX = -1.0 * (nX + m_XDispl) / m_XScale;
    else
        dX = (nX - m_XDispl) / m_XScale;

[Even Rouault]

UBSAN only warns about divison by zero I think. If other non-sensical values are stored in m_XScale and m_YScale, dX or dY can still be computed.

[Kurt Schwehr]

So it's okay to keep going with an inf or NaN dx or dy?

[Even Rouault]

You could only get Inf or Nan now if m_XDisplay is itself Inf or Nan, right ? Ah, actually if m_XScale is NaN, the result of the division is likely NaN too.

Anyway, if we get such non-sensical coordinates, there are a lot of drivers (think to shapefile) that happily directly take double values for cooordinates from the file and put them as coordinates of OGRGeometry's. So this would be more a general problem, if it is one (the question would be: is it the responsibility of GDAL or the caller to do sanitization of the geometry coordinates ?)

[Kurt Schwehr]

the question would be: is it the responsibility of GDAL or the caller to do sanitization of the geometry coordinates?

That is the same question I have. Maybe it is best to do it in the core so that all drivers are protected the same way. But any driver doing math on those numbers needs to be careful with the math.

This keeps coming up again and again across many libraries.

Several of us have been thinking there should probably make a port/cpl_safemath.cpp or something with things like this. Write it once and test it like crazy.

// UNTESTED and likely has some bugs...

template <typename T> T CPL_Multiply(T a, T b, bool *overflow /* or under */ ) {
  // ...
}

template <typename T> T CPL_Divide(T a, T b, bool *overflow) {
  // ...
}

template <typename T> T CPL_Add(T a, T b, bool *overflow) {
  // ...
}


template <typename T1, typename T2>
T2 CPL_SafeFloatToIntegerCast(T1 fNumber, bool *ok)
{
    CPLAssert(ok != NULL);
    if( fNumber >= static_cast<T1>(std::numeric_limits<T2>::max()) ||
        fNumber <= static_cast<T1>(std::numeric_limits<T2>::min()) ||
        CPLIsNan(fNumber) )
    {
        // How to do the printf string correctly?
        CPLError(CE_Failure, CPLE_AppDefined,
                 "Too large for new type: %lf", static_cast<double>(fNumber));
        *ok = false;
        return 0;
    }
    *ok = true;
    return static_cast<T2>(fNumber);
}

// Silent with clamping - overload args or change the name?
template <typename T1, typename T2>
T2 CPL_SafeFloatToIntegerCast(T1 fNumber)
{
  // ...
}

template <typename T1, typename T2>
T2 CPL_SafeNarrowCast(T1 dNumber, bool *ok)
{
    CPLAssert(ok != NULL);
    if( dNumber > static_cast<T1>(std::numeric_limits<T2>::max()) ||
        dNumber < static_cast<T1>(std::numeric_limits<T2>::min()) )
    {
        // How to do the printf string correctly?
        CPLError(CE_Failure, CPLE_AppDefined,
                 "Too large for new type: %ld", static_cast<GInt64>(dNumber));
        *ok = false;
        return 0;
    }
    *ok = true;
    return static_cast<T2>(dNumber);
}

// Silent with clamping - overload args or change the name?
template <typename T1, typename T2>
T2 CPL_SafeNarrowCast(T1 dNumber)
{
    CPLAssert(ok != NULL);
    if( dNumber >= static_cast<T1>(std::numeric_limits<T2>::max()) )
        return std::numeric_limits<T2>::max();
    if( dNumber <= static_cast<T1>(std::numeric_limits<T2>::min()) )
        return std::numeric_limits<T2>::min();
    return static_cast<T2>(dNumber);
}

[Even Rouault]

I guess we can close the ticket. Your latest comment is a more general issue, and I'm wondering if there's not already a ticket about it