From f4a1fab9a3ecbdbc283daf1b54eddf70a01bddd1 Mon Sep 17 00:00:00 2001 From: Stefan Goetschi Date: Tue, 3 Sep 2024 22:15:03 +0200 Subject: [PATCH] Fix undefined behavior in union. 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 --- osgplugin/ReaderWriterCityGML.cpp | 12 ++++++------ sources/include/citygml/vecs.hpp | 29 ++--------------------------- sources/src/citygml/envelope.cpp | 4 ++-- sources/src/citygml/tesselator.cpp | 2 +- 4 files changed, 11 insertions(+), 36 deletions(-) diff --git a/osgplugin/ReaderWriterCityGML.cpp b/osgplugin/ReaderWriterCityGML.cpp index 987a9fa2..1cb621be 100644 --- a/osgplugin/ReaderWriterCityGML.cpp +++ b/osgplugin/ReaderWriterCityGML.cpp @@ -526,16 +526,16 @@ void setMaterial(osg::ref_ptr 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() ); diff --git a/sources/include/citygml/vecs.hpp b/sources/include/citygml/vecs.hpp index f1cf4487..ad7c80a8 100644 --- a/sources/include/citygml/vecs.hpp +++ b/sources/include/citygml/vecs.hpp @@ -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; @@ -148,9 +141,6 @@ template< class T > class TVec3 inline bool operator==( const TVec3& rhs ) const; inline bool operator!=( const TVec3& rhs ) const; - - inline operator T*() { return xyz; } - inline operator const T*() const { return xyz; } }; template< class T > inline TVec3::TVec3( const T x, const T y, const T z ) @@ -160,11 +150,6 @@ template< class T > inline TVec3::TVec3( const T x, const T y, const T z ) this->z = z; } -template< class T > inline TVec3::TVec3( const T vec[] ) -{ - memcpy( xyz, vec, 3 * sizeof(T) ); -} - template< class T > inline T TVec3::length() const { return (T)sqrt( x*x + y*y + z*z ); @@ -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 ) @@ -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 inline std::ostream& operator<<( std::ostream & os, TVec4 const & v ) diff --git a/sources/src/citygml/envelope.cpp b/sources/src/citygml/envelope.cpp index b8c6de38..4ba501d3 100644 --- a/sources/src/citygml/envelope.cpp +++ b/sources/src/citygml/envelope.cpp @@ -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 ) diff --git a/sources/src/citygml/tesselator.cpp b/sources/src/citygml/tesselator.cpp index 9d2d0bf6..23404987 100644 --- a/sources/src/citygml/tesselator.cpp +++ b/sources/src/citygml/tesselator.cpp @@ -87,7 +87,7 @@ void Tesselator::processContours() for ( unsigned int i = 0; i < contour.length; i++ ) { void* data = reinterpret_cast(static_cast(_indices[contour.index + i])); - gluTessVertex( _tobj, &(_originalVertices[contour.index + i][0]), data ); + gluTessVertex( _tobj, &(_originalVertices[contour.index + i].x), data ); } gluTessEndContour( _tobj );