Skip to content

Commit

Permalink
Fix undefined behavior in union.
Browse files Browse the repository at this point in the history
Accessing non-active members in unions is undefined
behavior. Additionally the C++ Standard doesn't
support anonymous structs. [1]
This puts the current implementation of TVec3 and
TVec4 square into undefined behavior territory.

Removing the unions and anonymous structs is easy
and fixes all problematic paths.

[1]: https://stackoverflow.com/questions/2253878/why-does-c-disallow-anonymous-structs
  • Loading branch information
gostefan committed Sep 5, 2024
1 parent e85f164 commit 138a659
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 36 deletions.
12 changes: 6 additions & 6 deletions osgplugin/ReaderWriterCityGML.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -526,16 +526,16 @@ void setMaterial(osg::ref_ptr<osg::StateSet> stateset, const citygml::Polygon& p
return;
}

TVec4f diffuse( citygmlMaterial->getDiffuse(), 0.f );
TVec4f emissive( citygmlMaterial->getEmissive(), 0.f );
TVec4f specular( citygmlMaterial->getSpecular(), 0.f );
TVec3f diffuse = citygmlMaterial->getDiffuse();
TVec3f emissive = citygmlMaterial->getEmissive();
TVec3f specular = citygmlMaterial->getSpecular();
float ambient = citygmlMaterial->getAmbientIntensity();

osg::Material* material = new osg::Material;
material->setColorMode( osg::Material::OFF );
material->setDiffuse( osg::Material::FRONT_AND_BACK, osg::Vec4(diffuse.r, diffuse.g, diffuse.b, diffuse.a ) );
material->setSpecular( osg::Material::FRONT_AND_BACK, osg::Vec4(specular.r, specular.g, specular.b, specular.a ) );
material->setEmission( osg::Material::FRONT_AND_BACK, osg::Vec4(emissive.r, emissive.g, emissive.b, emissive.a ) );
material->setDiffuse( osg::Material::FRONT_AND_BACK, osg::Vec4(diffuse.x, diffuse.y, diffuse.z, 0.f ) );
material->setSpecular( osg::Material::FRONT_AND_BACK, osg::Vec4(specular.x, specular.y, specular.z, 0.f ) );
material->setEmission( osg::Material::FRONT_AND_BACK, osg::Vec4(emissive.x, emissive.y, emissive.z, 0.f ) );
material->setShininess( osg::Material::FRONT_AND_BACK, 128.f * citygmlMaterial->getShininess() );
material->setAmbient( osg::Material::FRONT_AND_BACK, osg::Vec4( ambient, ambient, ambient, 1.0 ) );
material->setTransparency( osg::Material::FRONT_AND_BACK, citygmlMaterial->getTransparency() );
Expand Down
29 changes: 2 additions & 27 deletions sources/include/citygml/vecs.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,10 @@ typedef TVec2< double > TVec2d;
template< class T > class TVec3
{
public:
union
{
T xyz[3];
T rgb[3];
struct { T x, y, z; };
struct { T r, g, b; };
};
T x, y, z;

public:
TVec3( const T x = (T)0, const T y = (T)0, const T z = (T)0 );
TVec3( const T vec[] );

inline T length() const;
inline T sqrLength() const;
Expand All @@ -148,9 +141,6 @@ template< class T > class TVec3

inline bool operator==( const TVec3<T>& rhs ) const;
inline bool operator!=( const TVec3<T>& rhs ) const;

inline operator T*() { return xyz; }
inline operator const T*() const { return xyz; }
};

template< class T > inline TVec3<T>::TVec3( const T x, const T y, const T z )
Expand All @@ -160,11 +150,6 @@ template< class T > inline TVec3<T>::TVec3( const T x, const T y, const T z )
this->z = z;
}

template< class T > inline TVec3<T>::TVec3( const T vec[] )
{
memcpy( xyz, vec, 3 * sizeof(T) );
}

template< class T > inline T TVec3<T>::length() const
{
return (T)sqrt( x*x + y*y + z*z );
Expand Down Expand Up @@ -301,13 +286,7 @@ typedef TVec3< double > TVec3d;
template< class T > class TVec4
{
public:
union
{
T xyzw[4];
T rgba[4];
struct { T x, y, z, w; };
struct { T r, g, b, a; };
};
T x, y, z, w;

public:
TVec4( const T x = (T)0, const T y = (T)0, const T z = (T)0, const T w = (T)0 )
Expand All @@ -317,10 +296,6 @@ template< class T > class TVec4
this->z = z;
this->w = w;
}

TVec4( const T vec[], const T w ) { memcpy( xyzw, vec, 4 * sizeof(T) ); this->w = w; }

TVec4( const T vec[] ) { memcpy( xyzw, vec, 4 * sizeof(T) ); }
};

template<class T> inline std::ostream& operator<<( std::ostream & os, TVec4<T> const & v )
Expand Down
4 changes: 2 additions & 2 deletions sources/src/citygml/envelope.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ namespace citygml {

const bool Envelope::validBounds() const
{
return !(ISNAN(m_lowerBound[0]) || ISNAN(m_lowerBound[1]) || ISNAN(m_lowerBound[2])
|| ISNAN(m_upperBound[0]) || ISNAN(m_upperBound[1]) || ISNAN(m_upperBound[2]));
return !(ISNAN(m_lowerBound.x) || ISNAN(m_lowerBound.y) || ISNAN(m_lowerBound.z)
|| ISNAN(m_upperBound.x) || ISNAN(m_upperBound.y) || ISNAN(m_upperBound.z));
}

std::ostream& operator<<( std::ostream& os, const Envelope& e )
Expand Down
2 changes: 1 addition & 1 deletion sources/src/citygml/tesselator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ void Tesselator::processContours()
for ( unsigned int i = 0; i < contour.length; i++ )
{
void* data = reinterpret_cast<void*>(static_cast<uintptr_t>(_indices[contour.index + i]));
gluTessVertex( _tobj, &(_originalVertices[contour.index + i][0]), data );
gluTessVertex( _tobj, &(_originalVertices[contour.index + i].x), data );
}

gluTessEndContour( _tobj );
Expand Down

0 comments on commit 138a659

Please sign in to comment.