kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #16859
[PATCH] vrml_v2_modelparser coverity fix + check
1. Someone recently touched vrml_v2_modelparser and made the same
mistake that was fixed a few weeks ago, i.e. declared array being
compared to NULL which is always true.
2. GetNextTag itself is kind of "flawed" and coverity doesn't catch
it. text buffer is allocated by the calling function but GetNextTag
can overflow the buffer reading because it doesn't know the buffer
size. Patch passes buffer size to GetNextTag.
3. All usage of GetNextTag treats the return type as a bool rather
than the pointer. Let's declare it boolean in that case instead of
returning the pointer to the same buffer we are passing as
argument....
From 410c30db3a435c023fa93e9e9e38166263fd8287 Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Tue, 17 Feb 2015 20:33:01 -0500
Subject: [PATCH 1/2] Fix vrml_v2_modelparser.cpp array comparisons
---
3d-viewer/vrml_v2_modelparser.cpp | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/3d-viewer/vrml_v2_modelparser.cpp b/3d-viewer/vrml_v2_modelparser.cpp
index 7cb9c3f..4d8618f 100644
--- a/3d-viewer/vrml_v2_modelparser.cpp
+++ b/3d-viewer/vrml_v2_modelparser.cpp
@@ -262,10 +262,10 @@ int VRML2_MODEL_PARSER::read_DEF_Coordinate()
while( GetNextTag( m_file, text ) )
{
- if( ( text == NULL ) || ( *text == ']' ) )
+ if( *text == ']' )
continue;
- if( ( *text == '}' ) )
+ if( *text == '}' )
return 0;
if( strcmp( text, "Coordinate" ) == 0 )
@@ -669,10 +669,10 @@ int VRML2_MODEL_PARSER::read_IndexedLineSet()
while( GetNextTag( m_file, text ) )
{
- if( ( text == NULL ) || ( *text == ']' ) )
+ if( *text == ']' )
continue;
- if( ( *text == '}' ) )
+ if( *text == '}' )
return 0;
if( strcmp( text, "Coordinate" ) == 0 )
@@ -874,10 +874,10 @@ int VRML2_MODEL_PARSER::read_CoordinateDef()
while( GetNextTag( m_file, text ) )
{
- if( ( text == NULL ) || ( *text == ']' ) )
+ if( *text == ']' )
continue;
- if( ( *text == '}' ) )
+ if( *text == '}' )
return 0;
if( strcmp( text, "point" ) == 0 )
--
1.9.1
From 358b31f3c1618261ae93d01aba3555fd9bc6af9e Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Tue, 17 Feb 2015 20:52:59 -0500
Subject: [PATCH 2/2] Make GetNextTag implementation safer by checking number
of bytes read, also use bool return type as the function was used as such in
all cases
---
3d-viewer/vrml_aux.cpp | 9 ++++----
3d-viewer/vrml_aux.h | 2 +-
3d-viewer/vrml_v1_modelparser.cpp | 10 ++++-----
3d-viewer/vrml_v2_modelparser.cpp | 44 +++++++++++++++++++--------------------
4 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/3d-viewer/vrml_aux.cpp b/3d-viewer/vrml_aux.cpp
index 8422efa..0504a65 100644
--- a/3d-viewer/vrml_aux.cpp
+++ b/3d-viewer/vrml_aux.cpp
@@ -94,13 +94,13 @@ static int SkipGetChar( FILE* File )
}
-char* GetNextTag( FILE* File, char* tag )
+bool GetNextTag( FILE* File, char* tag, size_t len )
{
int c = SkipGetChar( File );
if( c == EOF )
{
- return NULL;
+ return false;
}
tag[0] = c;
@@ -111,7 +111,7 @@ char* GetNextTag( FILE* File, char* tag )
{
char* dst = &tag[1];
- while( fscanf( File, "%c", dst ) )
+ while( fscanf( File, "%c", dst ) && len > 0 )
{
if( (*dst == ' ') || (*dst == '[') || (*dst == '{')
|| (*dst == '\t') || (*dst == '\n')|| (*dst == '\r') )
@@ -121,6 +121,7 @@ char* GetNextTag( FILE* File, char* tag )
}
dst++;
+ len--;
}
@@ -134,7 +135,7 @@ char* GetNextTag( FILE* File, char* tag )
}
}
- return tag;
+ return true;
}
diff --git a/3d-viewer/vrml_aux.h b/3d-viewer/vrml_aux.h
index f245ce7..718ad72 100644
--- a/3d-viewer/vrml_aux.h
+++ b/3d-viewer/vrml_aux.h
@@ -52,6 +52,6 @@ int read_NotImplemented( FILE* File, char closeChar);
int parseVertexList( FILE* File, std::vector< glm::vec3 > &dst_vector);
int parseVertex( FILE* File, glm::vec3 &dst_vertex );
int parseFloat( FILE* File, float *dst_float );
-char* GetNextTag( FILE* File, char* tag );
+bool GetNextTag( FILE* File, char* tag, size_t len );
#endif
diff --git a/3d-viewer/vrml_v1_modelparser.cpp b/3d-viewer/vrml_v1_modelparser.cpp
index ee7a2fd..97cc69b 100644
--- a/3d-viewer/vrml_v1_modelparser.cpp
+++ b/3d-viewer/vrml_v1_modelparser.cpp
@@ -88,7 +88,7 @@ void VRML1_MODEL_PARSER::Load( const wxString& aFilename, double aVrmlunits_to_3
childs.clear();
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( ( *text == '}' ) || ( *text == ']' ) )
{
@@ -123,7 +123,7 @@ int VRML1_MODEL_PARSER::read_separator()
// DBG( printf( "Separator\n" ) );
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( strcmp( text, "Material" ) == 0 )
{
@@ -182,7 +182,7 @@ int VRML1_MODEL_PARSER::readMaterial()
m_model->m_Materials = material;
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -230,7 +230,7 @@ int VRML1_MODEL_PARSER::readCoordinate3()
// DBG( printf( " readCoordinate3\n" ) );
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -258,7 +258,7 @@ int VRML1_MODEL_PARSER::readIndexedFaceSet()
// DBG( printf( " readIndexedFaceSet\n" ) );
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
diff --git a/3d-viewer/vrml_v2_modelparser.cpp b/3d-viewer/vrml_v2_modelparser.cpp
index 4d8618f..97ba12c 100644
--- a/3d-viewer/vrml_v2_modelparser.cpp
+++ b/3d-viewer/vrml_v2_modelparser.cpp
@@ -99,7 +99,7 @@ void VRML2_MODEL_PARSER::Load( const wxString& aFilename, double aVrmlunits_to_3
childs.clear();
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( ( *text == '}' ) || ( *text == ']' ) )
{
@@ -138,7 +138,7 @@ int VRML2_MODEL_PARSER::read_Transform()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -257,10 +257,10 @@ int VRML2_MODEL_PARSER::read_DEF_Coordinate()
char text[128];
// Get the name of the definition.
- GetNextTag( m_file, text );
+ GetNextTag( m_file, text, sizeof(text) );
std::string coordinateName = text;
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
continue;
@@ -287,9 +287,9 @@ int VRML2_MODEL_PARSER::read_DEF()
{
char text[128];
- GetNextTag( m_file, text );
+ GetNextTag( m_file, text, sizeof(text) );
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -344,7 +344,7 @@ int VRML2_MODEL_PARSER::read_USE()
char text[128];
// Get the name of the definition.
- GetNextTag( m_file, text );
+ GetNextTag( m_file, text, sizeof(text) );
std::string coordinateName = text;
// Look for it in our coordinate map.
@@ -368,7 +368,7 @@ int VRML2_MODEL_PARSER::read_Shape()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -418,7 +418,7 @@ int VRML2_MODEL_PARSER::read_Appearance()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -446,7 +446,7 @@ int VRML2_MODEL_PARSER::read_material()
S3D_MATERIAL* material = NULL;
char text[128];
- if( GetNextTag( m_file, text ) )
+ if( GetNextTag( m_file, text, sizeof(text) ) )
{
if( strcmp( text, "Material" ) == 0 )
{
@@ -462,7 +462,7 @@ int VRML2_MODEL_PARSER::read_material()
}
else if( strcmp( text, "DEF" ) == 0 )
{
- if( GetNextTag( m_file, text ) )
+ if( GetNextTag( m_file, text, sizeof(text) ) )
{
wxString mat_name;
mat_name = FROM_UTF8( text );
@@ -471,7 +471,7 @@ int VRML2_MODEL_PARSER::read_material()
GetMaster()->Insert( material );
m_model->m_Materials = material;
- if( GetNextTag( m_file, text ) )
+ if( GetNextTag( m_file, text, sizeof(text) ) )
{
if( strcmp( text, "Material" ) == 0 )
{
@@ -482,7 +482,7 @@ int VRML2_MODEL_PARSER::read_material()
}
else if( strcmp( text, "USE" ) == 0 )
{
- if( GetNextTag( m_file, text ) )
+ if( GetNextTag( m_file, text, sizeof(text) ) )
{
wxString mat_name;
mat_name = FROM_UTF8( text );
@@ -511,7 +511,7 @@ int VRML2_MODEL_PARSER::read_Material()
char text[128];
glm::vec3 vertex;
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -593,7 +593,7 @@ int VRML2_MODEL_PARSER::read_IndexedFaceSet()
m_normalPerVertex = false;
colorPerVertex = false;
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -607,7 +607,7 @@ int VRML2_MODEL_PARSER::read_IndexedFaceSet()
if( strcmp( text, "normalPerVertex" ) == 0 )
{
- if( GetNextTag( m_file, text ) )
+ if( GetNextTag( m_file, text, sizeof(text) ) )
{
if( strcmp( text, "TRUE" ) == 0 )
{
@@ -617,7 +617,7 @@ int VRML2_MODEL_PARSER::read_IndexedFaceSet()
}
else if( strcmp( text, "colorPerVertex" ) == 0 )
{
- GetNextTag( m_file, text );
+ GetNextTag( m_file, text, sizeof(text) );
if( strcmp( text, "TRUE" ) )
{
@@ -667,7 +667,7 @@ int VRML2_MODEL_PARSER::read_IndexedLineSet()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
continue;
@@ -783,7 +783,7 @@ int VRML2_MODEL_PARSER::read_Color()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -810,7 +810,7 @@ int VRML2_MODEL_PARSER::read_Normal()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -843,7 +843,7 @@ int VRML2_MODEL_PARSER::read_Coordinate()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
{
@@ -872,7 +872,7 @@ int VRML2_MODEL_PARSER::read_CoordinateDef()
{
char text[128];
- while( GetNextTag( m_file, text ) )
+ while( GetNextTag( m_file, text, sizeof(text) ) )
{
if( *text == ']' )
continue;
--
1.9.1
Follow ups
