Opened 2 years ago

Closed 2 years ago

#6889 closed defect (fixed)

mitab_mapheaderblock.cpp XScale and YScale in r38361

Reported by: Kurt Schwehr Owned by: warmerdam
Priority: normal Milestone:
Component: default Version: unspecified
Severity: normal Keywords: fuzzing floatingpoint mitab
Cc:

Description

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;

Change History (5)

comment:1 Changed 2 years ago by 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.

comment:2 Changed 2 years ago by Kurt Schwehr

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

comment:3 Changed 2 years ago by 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 ?)

comment:4 Changed 2 years ago by 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);
}

comment:5 Changed 2 years ago by Even Rouault

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.