Skip to content
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.

Sign up for GitHub

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

Fix potential undefined behaviour (divide by zero) #224

Merged
merged 1 commit into from
Jan 22, 2023
Merged

Fix potential undefined behaviour (divide by zero) #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/E57Exception.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ namespace e57
/// an invalid node type was passed in Data3D pointFields
ErrorInvalidNodeType = 51,

/// passed an invalid value in Data3D pointFields
ErrorInvalidData3DValue = 52,

/// @deprecated Will be removed in 4.0. Use e57::Success.
E57_SUCCESS DEPRECATED_ENUM( "Will be removed in 4.0. Use Success." ) = Success,
/// @deprecated Will be removed in 4.0. Use e57::ErrorBadCVHeader.
Expand Down
9 changes: 6 additions & 3 deletions src/CheckedFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,6 @@ void CheckedFile::read( char *buf, size_t nRead, size_t /*bufSize*/ )
std::vector<char> page_buffer_v( physicalPageSize );
char *page_buffer = page_buffer_v.data();

const auto checksumMod = static_cast<unsigned int>( std::nearbyint( 100.0 / checkSumPolicy_ ) );

while ( nRead > 0 )
{
readPhysicalPage( page_buffer, page );
Expand All @@ -327,11 +325,16 @@ void CheckedFile::read( char *buf, size_t nRead, size_t /*bufSize*/ )
break;

default:
{
const auto checksumMod =
static_cast<unsigned int>( std::nearbyint( 100.0 / checkSumPolicy_ ) );

if ( !( page % checksumMod ) || ( nRead < physicalPageSize ) )
{
verifyChecksum( page_buffer, page );
}
break;
}
break;
}

memcpy( buf, page_buffer + pageOffset, n );
Expand Down
2 changes: 2 additions & 0 deletions src/E57Exception.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,8 @@ namespace e57
return "class invariance constraint violation in debug mode (ErrorInvarianceViolation)";
case ErrorInvalidNodeType:
return "an invalid node type was passed in Data3D pointFields";
case ErrorInvalidData3DValue:
return "an invalid value was passed in Data3D pointFields";

default:
return "unknown error (" + std::to_string( ecode ) + ")";
Expand Down
47 changes: 29 additions & 18 deletions src/WriterImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,19 +828,9 @@ namespace e57
// "/data3D/0/points/0/cartesianX"
StructureNode proto( imf_ );

// Because ScaledInteger min/max are the raw integer min/max, we must calculate them from the
// data min/max
const double pointRangeScale = data3DHeader.pointFields.pointRangeScale;
const double pointRangeOffset = 0.0;

const double pointRangeMin = data3DHeader.pointFields.pointRangeMinimum;
const double pointRangeMax = data3DHeader.pointFields.pointRangeMaximum;

const auto pointRangeMinimum = static_cast<int64_t>(
std::floor( ( pointRangeMin - pointRangeOffset ) / pointRangeScale + .5 ) );
const auto pointRangeMaximum = static_cast<int64_t>(
std::floor( ( pointRangeMax - pointRangeOffset ) / pointRangeScale + .5 ) );

const auto getPointProto = [=]() -> Node {
switch ( data3DHeader.pointFields.pointRangeNodeType )
{
Expand All @@ -851,6 +841,21 @@ namespace e57

case NumericalNodeType::ScaledInteger:
{
// Because ScaledInteger min/max are the raw integer min/max, we must calculate them
// from the data min/max
const double pointRangeOffset = 0.0;
const double pointRangeScale = data3DHeader.pointFields.pointRangeScale;

if ( pointRangeScale == 0.0 )
{
throw E57_EXCEPTION2( ErrorInvalidData3DValue, "pointRangeScale cannot be 0" );
}

const auto pointRangeMinimum = static_cast<int64_t>(
std::floor( ( pointRangeMin - pointRangeOffset ) / pointRangeScale + .5 ) );
const auto pointRangeMaximum = static_cast<int64_t>(
std::floor( ( pointRangeMax - pointRangeOffset ) / pointRangeScale + .5 ) );

return ScaledIntegerNode( imf_, 0, pointRangeMinimum, pointRangeMaximum,
pointRangeScale, pointRangeOffset );
}
Expand All @@ -876,6 +881,7 @@ namespace e57
{
proto.set( "cartesianX", getPointProto() );
}

if ( data3DHeader.pointFields.cartesianYField )
{
proto.set( "cartesianY", getPointProto() );
Expand All @@ -894,14 +900,6 @@ namespace e57
const double angleMin = data3DHeader.pointFields.angleMinimum;
const double angleMax = data3DHeader.pointFields.angleMaximum;

const double angleScale = data3DHeader.pointFields.angleScale;
const double angleOffset = 0.0;

const auto angleMinimum =
static_cast<int64_t>( std::floor( ( angleMin - angleOffset ) / angleScale + .5 ) );
const auto angleMaximum =
static_cast<int64_t>( std::floor( ( angleMax - angleOffset ) / angleScale + .5 ) );

const auto getAngleProto = [=]() -> Node {
switch ( data3DHeader.pointFields.angleNodeType )
{
Expand All @@ -912,6 +910,19 @@ namespace e57

case NumericalNodeType::ScaledInteger:
{
const double angleOffset = 0.0;
const double angleScale = data3DHeader.pointFields.angleScale;

if ( angleScale == 0.0 )
{
throw E57_EXCEPTION2( ErrorInvalidData3DValue, "angleScale cannot be 0" );
}

const auto angleMinimum = static_cast<int64_t>(
std::floor( ( angleMin - angleOffset ) / angleScale + .5 ) );
const auto angleMaximum = static_cast<int64_t>(
std::floor( ( angleMax - angleOffset ) / angleScale + .5 ) );

return ScaledIntegerNode( imf_, 0, angleMinimum, angleMaximum, angleScale,
angleOffset );
}
Expand Down
56 changes: 56 additions & 0 deletions test/src/test_SimpleWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,62 @@ TEST( SimpleWriter, Empty )
E57_ASSERT_NO_THROW( e57::Writer writer( "./empty.e57", options ) );
}

TEST( SimpleWriter, InvalidData3DValueCartesian )
{
e57::WriterOptions options;
options.guid = "Invalid Data3D Value GUID";

e57::Writer *writer = nullptr;

E57_ASSERT_NO_THROW( writer = new e57::Writer( "./InvalidData3DValue.e57", options ) );

constexpr uint16_t cNumPoints = 1;

e57::Data3D header;
header.guid = "Invalid Data3D Value Header GUID";
header.pointCount = cNumPoints;
header.pointFields.cartesianXField = true;
header.pointFields.cartesianYField = true;
header.pointFields.cartesianZField = true;
header.pointFields.pointRangeNodeType = e57::NumericalNodeType::ScaledInteger;

// since we are requesting a scaled int, leaving pointRangeScale as 0.0 should throw an exception

e57::Data3DPointsDouble pointsData( header );

E57_ASSERT_THROW( writer->WriteData3DData( header, pointsData ) );

delete writer;
}

TEST( SimpleWriter, InvalidData3DValueSpherical )
{
e57::WriterOptions options;
options.guid = "Invalid Data3D Value GUID";

e57::Writer *writer = nullptr;

E57_ASSERT_NO_THROW( writer = new e57::Writer( "./InvalidData3DValue.e57", options ) );

constexpr uint16_t cNumPoints = 1;

e57::Data3D header;
header.guid = "Invalid Data3D Value Header GUID";
header.pointCount = cNumPoints;
header.pointFields.sphericalRangeField = true;
header.pointFields.sphericalAzimuthField = true;
header.pointFields.sphericalElevationField = true;
header.pointFields.angleNodeType = e57::NumericalNodeType::ScaledInteger;

// since we are requesting a scaled int, leaving angleScale as 0.0 should throw an exception

e57::Data3DPointsDouble pointsData( header );

E57_ASSERT_THROW( writer->WriteData3DData( header, pointsData ) );

delete writer;
}

// Write a coloured cube of points using doubles.
TEST( SimpleWriter, ColouredCubeDouble )
{
Expand Down