kicad-developers team mailing list archive

[Michael Steinberg <michsteinb@xxxxxxxxx>]

The current implementation of LSET::FmtHex would access bits outside the 
range of of the bitset. I combined the fix with a code refactorization 
that clears up things in my opinion, which of course is the one that 
does not count ;) A save/reload cycle in debug builds that would crash 
does not crash anymore and seems to leave the data intact. Also 
introduce size_t for sizes and indices.

>From 7c4238af4811a281288290dd229b9250d22e1fe9 Mon Sep 17 00:00:00 2001
From: decimad <michsteinb@xxxxxxxxx>
Date: Tue, 5 Jul 2016 19:16:55 +0200
Subject: [PATCH] Fix bitset access outside boundaries in LSET::FmtHex().
 Refactor Fmt and Parse a bit to make them look more alike. Use size_t for
 indices and sizes.

---
 common/lset.cpp                           | 52 ++++++++++---------------------
 include/layers_id_colors_and_visibility.h |  4 +--
 pcbnew/pcb_plot_params.cpp                |  1 +
 3 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/common/lset.cpp b/common/lset.cpp
index 4e4248d..2996feb 100644
--- a/common/lset.cpp
+++ b/common/lset.cpp
@@ -25,6 +25,7 @@
 
 #include <stdarg.h>
 #include <assert.h>
+#include <algorithm>
 
 #include <layers_id_colors_and_visibility.h>
 #include <class_board.h>
@@ -229,9 +230,7 @@ std::string LSET::FmtBin() const
 {
     std::string ret;
 
-    int     bit_count = size();
-
-    for( int bit=0;  bit<bit_count;  ++bit )
+    for( int bit = 0;  bit < size();  ++bit )
     {
         if( bit )
         {
@@ -255,20 +254,15 @@ std::string LSET::FmtHex() const
 
     static const char hex[] = "0123456789abcdef";
 
-    int     nibble_count = ( size() + 3 ) / 4;
-
-    for( int nibble=0;  nibble<nibble_count;  ++nibble )
+    for( size_t bit = 0; bit < size(); bit += 4 )
     {
         unsigned ndx = 0;
 
         // test 4 consecutive bits and set ndx to 0-15:
-        for( int nibble_bit=0;  nibble_bit<4;  ++nibble_bit )
-        {
-            if( (*this)[nibble_bit + nibble*4] )
-                ndx |= (1 << nibble_bit);
-        }
+        for( int i = bit; i < std::min<int>( bit + 4, size() ); ++i )
+            ndx |= operator[]( i ) << (i & 3);
 
-        if( nibble && !( nibble % 8 ) )
+        if( bit && bit % (8 * 4) == 0 )
             ret += '_';
 
         assert( ndx < DIM( hex ) );
@@ -281,25 +275,21 @@ std::string LSET::FmtHex() const
 }
 
 
-int LSET::ParseHex( const char* aStart, int aCount )
+size_t LSET::ParseHex( const char* aStart, size_t aCount )
 {
     LSET tmp;
+    size_t index = aCount;
 
-    const char* rstart = aStart + aCount - 1;
-    const char* rend   = aStart - 1;
-
-    const int bitcount = size();
-
-    int nibble_ndx = 0;
-
-    while( rstart > rend )
+    /// @todo Even the old version parses "_" to a set of zeros.
+    for( size_t bit = 0; bit < size() && index > 0; )
     {
-        int cc = *rstart--;
+        // parse in reverse order!
+        unsigned int cc = aStart[--index];
 
         if( cc == '_' )
             continue;
 
-        int nibble;
+        unsigned int nibble;
 
         if( cc >= '0' && cc <= '9' )
             nibble = cc - '0';
@@ -310,21 +300,13 @@ int LSET::ParseHex( const char* aStart, int aCount )
         else
             break;
 
-        int bit = nibble_ndx * 4;
-
-        for( int ndx=0; bit<bitcount && ndx<4; ++bit, ++ndx )
-            if( nibble & (1<<ndx) )
-                tmp.set( bit );
+        for( size_t i = bit; i < std::min<size_t>( bit + 4, size() ); ++i )
+            tmp.set( i, (nibble & (1 << (i & 3))) != 0 );
 
-        if( bit >= bitcount )
-            break;
-
-        ++nibble_ndx;
+        bit += 4;
     }
 
-    int byte_count = aStart + aCount - 1 - rstart;
-
-    assert( byte_count >= 0 );
+    size_t byte_count = aCount - index;
 
     if( byte_count > 0 )
         *this = tmp;
diff --git a/include/layers_id_colors_and_visibility.h b/include/layers_id_colors_and_visibility.h
index db72495..a59537c 100644
--- a/include/layers_id_colors_and_visibility.h
+++ b/include/layers_id_colors_and_visibility.h
@@ -365,9 +365,9 @@ public:
      * with those given in the input string.  Parsing stops at the first
      * non hex ASCII byte, except that marker bytes output from FmtHex are
      * not terminators.
-     * @return int - number of bytes consumed
+     * @return size_t - number of bytes consumed
      */
-    int ParseHex( const char* aStart, int aCount );
+    size_t ParseHex( const char* aStart, size_t aCount );
 
     /**
      * Function FmtBin
diff --git a/pcbnew/pcb_plot_params.cpp b/pcbnew/pcb_plot_params.cpp
index ffcf96e..16104e0 100644
--- a/pcbnew/pcb_plot_params.cpp
+++ b/pcbnew/pcb_plot_params.cpp
@@ -369,6 +369,7 @@ void PCB_PLOT_PARAMS_PARSER::Parse( PCB_PLOT_PARAMS* aPcbPlotParams )
                 else if( cur.find_first_of( "0x" ) == 0 )   // pretty ver. 4.
                 {
                     // skip the leading 2 0x bytes.
+                    assert( cur.size() >= 2 );  // implicit assumption now explicit.
                     aPcbPlotParams->m_layerSelection.ParseHex( cur.c_str()+2, cur.size()-2 );
                 }
                 else
-- 
2.9.0.windows.1