kicad-developers team mailing list archive

[Mark Roszko <mark.roszko@xxxxxxxxx>]

This fixes come from running static analysis on kicad using Coverity while
trying to hunt something down.


1 - fgetc in 3d-viewer/vrml_aux.cpp had it's return type used as char but
libc defines it as int. EOF can be -1 (32-bit signed) and thus would get
truncated. This is OS dependent.
2 - The error path in bitmap2componenet exits early on error without
freeing allocated memory
3 - GetStyleCodeIndex can get negative (wX_NOT_FOUND) returned. Check the
value before trying to access an array using it.
4 - Two strncpy calls that didn't have their buffers null terminated.
(strncpy won't set null if bytes available > limit).

From 40386fb70d46a982fbcd11b9c5b517c4291bac48 Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Sat, 17 Jan 2015 12:28:10 -0500
Subject: [PATCH 1/4] fgetc does not gurantee a char return type, especially
 for EOF which can be the full 32-bit type -1 on some OSes. Also statictize
 SkipGetChar as its only used by the functions in the file

---
 3d-viewer/vrml_aux.cpp | 16 ++++++++++------
 3d-viewer/vrml_aux.h   |  1 -
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/3d-viewer/vrml_aux.cpp b/3d-viewer/vrml_aux.cpp
index 7e34df1..8422efa 100644
--- a/3d-viewer/vrml_aux.cpp
+++ b/3d-viewer/vrml_aux.cpp
@@ -2,7 +2,7 @@
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
  * Copyright (C) 2014 Mario Luzeiro <mrluzeiro@xxxxxxxxx>
- * Copyright (C) 1992-2014 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -28,9 +28,13 @@
 
 #include "vrml_aux.h"
 
-char SkipGetChar( FILE* File )
+
+static int SkipGetChar ( FILE* File );
+
+
+static int SkipGetChar( FILE* File )
 {
-    char    c;
+    int    c;
     bool    re_parse;
 
     if( ( c = fgetc( File ) ) == EOF )
@@ -92,7 +96,7 @@ char SkipGetChar( FILE* File )
 
 char* GetNextTag( FILE* File, char* tag )
 {
-    char c = SkipGetChar( File );
+    int c = SkipGetChar( File );
 
     if( c == EOF )
     {
@@ -136,7 +140,7 @@ char* GetNextTag( FILE* File, char* tag )
 
 int read_NotImplemented( FILE* File, char closeChar )
 {
-    char c;
+    int c;
 
     // DBG( printf( "look for %c\n", closeChar) );
     while( ( c = fgetc( File ) ) != EOF )
@@ -189,7 +193,7 @@ int parseVertex( FILE* File, glm::vec3& dst_vertex )
     dst_vertex.y    = b;
     dst_vertex.z    = c;
 
-    char s = SkipGetChar( File );
+    int s = SkipGetChar( File );
 
     if( s != EOF )
     {
diff --git a/3d-viewer/vrml_aux.h b/3d-viewer/vrml_aux.h
index 479a403..f245ce7 100644
--- a/3d-viewer/vrml_aux.h
+++ b/3d-viewer/vrml_aux.h
@@ -49,7 +49,6 @@
 #include <wx/glcanvas.h>
 
 int read_NotImplemented( FILE* File, char closeChar);
-char SkipGetChar ( FILE* File );
 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 );
-- 
1.9.1

From 3a43853daecb2a608a8f63b69ce8845fe181ffa3 Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Sat, 17 Jan 2015 13:48:16 -0500
Subject: [PATCH 2/4] Fix memory leak in bitmap2component in error path

---
 bitmap2component/bitmap2component.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/bitmap2component/bitmap2component.cpp b/bitmap2component/bitmap2component.cpp
index 6e3427e..a8799ac 100644
--- a/bitmap2component/bitmap2component.cpp
+++ b/bitmap2component/bitmap2component.cpp
@@ -158,6 +158,12 @@ int bitmap2component( potrace_bitmap_t* aPotrace_bitmap, FILE* aOutfile,
     st = potrace_trace( param, aPotrace_bitmap );
     if( !st || st->status != POTRACE_STATUS_OK )
     {
+        if( st )
+        {
+            potrace_state_free( st );
+        }
+        potrace_param_free( param );
+
         fprintf( stderr, "Error tracing bitmap: %s\n", strerror( errno ) );
         return 1;
     }
-- 
1.9.1

From ba4e3991354d937bba032f6b2e83f0a7ec7952a6 Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Sat, 17 Jan 2015 14:07:49 -0500
Subject: [PATCH 3/4] GetStyleCodeIndex can be negative, don't index the array
 directly with it

---
 eeschema/lib_pin.cpp | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/eeschema/lib_pin.cpp b/eeschema/lib_pin.cpp
index f4d997a..409492d 100644
--- a/eeschema/lib_pin.cpp
+++ b/eeschema/lib_pin.cpp
@@ -1919,38 +1919,44 @@ void LIB_PIN::SetWidth( int aWidth )
 
 void LIB_PIN::GetMsgPanelInfo( MSG_PANEL_ITEMS& aList )
 {
-    wxString Text;
+    wxString text;
 
     LIB_ITEM::GetMsgPanelInfo( aList );
 
     aList.push_back( MSG_PANEL_ITEM( _( "Name" ), m_name, DARKCYAN ) );
 
     if( m_number == 0 )
-        Text = wxT( "?" );
+        text = wxT( "?" );
     else
-        PinStringNum( Text );
+        PinStringNum( text );
 
-    aList.push_back( MSG_PANEL_ITEM( _( "Number" ), Text, DARKCYAN ) );
+    aList.push_back( MSG_PANEL_ITEM( _( "Number" ), text, DARKCYAN ) );
 
     aList.push_back( MSG_PANEL_ITEM( _( "Type" ),
                                      wxGetTranslation( pin_electrical_type_names[ m_type ] ),
                                      RED ) );
-    Text = wxGetTranslation( pin_style_names[ GetStyleCodeIndex( m_shape ) ] );
-    aList.push_back( MSG_PANEL_ITEM( _( "Style" ), Text, BLUE ) );
+
+    int styleCodeIndex = GetStyleCodeIndex( m_shape );
+    if( styleCodeIndex >= 0 )
+        text = wxGetTranslation( pin_style_names[ styleCodeIndex ] );
+    else
+        text = wxT( "?" );
+
+    aList.push_back( MSG_PANEL_ITEM( _( "Style" ), text, BLUE ) );
 
     if( IsVisible() )
-        Text = _( "Yes" );
+        text = _( "Yes" );
     else
-        Text = _( "No" );
+        text = _( "No" );
 
-    aList.push_back( MSG_PANEL_ITEM( _( "Visible" ), Text, DARKGREEN ) );
+    aList.push_back( MSG_PANEL_ITEM( _( "Visible" ), text, DARKGREEN ) );
 
     // Display pin length
-    Text = StringFromValue( g_UserUnit, m_length, true );
-    aList.push_back( MSG_PANEL_ITEM( _( "Length" ), Text, MAGENTA ) );
+    text = StringFromValue( g_UserUnit, m_length, true );
+    aList.push_back( MSG_PANEL_ITEM( _( "Length" ), text, MAGENTA ) );
 
-    Text = wxGetTranslation( pin_orientation_names[ GetOrientationCodeIndex( m_orientation ) ] );
-    aList.push_back( MSG_PANEL_ITEM( _( "Orientation" ), Text, DARKMAGENTA ) );
+    text = wxGetTranslation( pin_orientation_names[ GetOrientationCodeIndex( m_orientation ) ] );
+    aList.push_back( MSG_PANEL_ITEM( _( "Orientation" ), text, DARKMAGENTA ) );
 }
 
 
@@ -2201,11 +2207,18 @@ BITMAP_DEF LIB_PIN::GetMenuImage() const
 wxString LIB_PIN::GetSelectMenuText() const
 {
     wxString tmp;
+    wxString style;
+
+    int styleCode = GetStyleCodeIndex( m_shape );
+    if( styleCode >= 0 )
+        style = wxGetTranslation( pin_style_names[ styleCode ] );
+    else
+        style = wxT( "?" );
 
     tmp.Printf( _( "Pin %s, %s, %s" ),
                 GetChars( GetNumberString() ),
                 GetChars( GetTypeString() ),
-                GetChars( wxGetTranslation( pin_style_names[ GetStyleCodeIndex( m_shape ) ] ) )
+                GetChars( style )
                 );
     return tmp;
 }
-- 
1.9.1

From 926ea802dd104be1161d45fc0c2dacb796cc26ef Mon Sep 17 00:00:00 2001
From: Mark Roszko <mark.roszko@xxxxxxxxx>
Date: Sat, 17 Jan 2015 14:39:14 -0500
Subject: [PATCH 4/4] Null terminate after strncpy to be safe

---
 pcbnew/legacy_netlist_reader.cpp | 1 +
 scripting/wx_python_helpers.cpp  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/pcbnew/legacy_netlist_reader.cpp b/pcbnew/legacy_netlist_reader.cpp
index f4973d6..cefe733 100644
--- a/pcbnew/legacy_netlist_reader.cpp
+++ b/pcbnew/legacy_netlist_reader.cpp
@@ -185,6 +185,7 @@ void LEGACY_NETLIST_READER::loadNet( char* aText, COMPONENT* aComponent ) throw(
     char     line[256];
 
     strncpy( line, aText, sizeof( line ) );
+    line[ sizeof(line) - 1 ] = '\0';
 
     if( ( p = strtok( line, " ()\t\n" ) ) == NULL )
     {
diff --git a/scripting/wx_python_helpers.cpp b/scripting/wx_python_helpers.cpp
index 8ee7bba..587fe2a 100644
--- a/scripting/wx_python_helpers.cpp
+++ b/scripting/wx_python_helpers.cpp
@@ -186,6 +186,7 @@ PyObject* wx2PyString( const wxString& src )
 void wxSetDefaultPyEncoding( const char* encoding )
 {
     strncpy( wxPythonEncoding, encoding, WX_DEFAULTENCODING_SIZE );
+    wxPythonEncoding[ WX_DEFAULTENCODING_SIZE - 1 ] = '\0';
 }
 
 
-- 
1.9.1