[RFC PATCH 0/2] fix overflow check for #basicAt: and #basicAt:put:

Holger found an overflow bug in #basicAt: and #basicAt:put:,
which breaks classes that have both fixed and indexed instance
variables.  The following patches fix it; I didn't yet add
testcases or attempted to deal with the fallout: as expected,
the delays.st test starts failing as it does under the JIT.
DebugTools also fails due to the same access with index -1.

Paolo

Paolo Bonzini (2):
  tweak index_oop_spec and index_oop_put_spec index computations
  fix overflow check for #basicAt: and #basicAt:put:

 libgst/ChangeLog | 15 +++++++++++++++
 libgst/dict.inl  | 44 ++++++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 20 deletions(-)

--
1.8.2.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

libgst:
2013-06-26  Paolo Bonzini  <[hidden email]>

        * libgst/dict.inl: In index_oop_spec and index_oop_put_spec,
        move index decrement after the scaling by sizeof(type) and the
        overflow test. This prepares for fixing the overflow test.
---
 libgst/ChangeLog |  6 ++++++
 libgst/dict.inl  | 20 ++++++++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index f5a77d7..893c380 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,3 +1,9 @@
+2013-06-26  Paolo Bonzini  <[hidden email]>
+
+ * libgst/dict.inl: In index_oop_spec and index_oop_put_spec,
+ move index decrement after the scaling by sizeof(type) and the
+ overflow test. This prepares for fixing the overflow test.
+
 2013-06-14  Gwenael Casaccio <[hidden email]>
 
  * libgst/dict.c: Remove useless code: gst_ordered_collection structure.
diff --git a/libgst/dict.inl b/libgst/dict.inl
index 434906a..3b029b0 100644
--- a/libgst/dict.inl
+++ b/libgst/dict.inl
@@ -985,8 +985,6 @@ index_oop_spec (OOP oop,
   if UNCOMMON (index < 1)
     return (NULL);
 
-  index--;
-
 #define DO_INDEX_OOP(type, dest) \
     /* Find the number of bytes in the object.  */ \
     maxByte = NUM_WORDS (object) * sizeof (PTR); \
@@ -998,9 +996,11 @@ index_oop_spec (OOP oop,
       + (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
  \
     /* Check that we're on bounds.  */ \
-    if UNCOMMON (index + sizeof(type) > maxByte) \
+    if UNCOMMON (index > maxByte) \
       return (NULL); \
  \
+    index -= sizeof(type); \
+ \
     /* Use a cast if unaligned accesses are supported, else memcpy.  */ \
     src = ((char *) object->data) + index; \
     if (sizeof (type) <= sizeof (PTR)) \
@@ -1085,10 +1085,10 @@ index_oop_spec (OOP oop,
       case GST_ISP_POINTER:
         maxIndex = NUM_WORDS (object);
         index += instanceSpec >> ISP_NUMFIXEDFIELDS;
-        if UNCOMMON (index >= maxIndex)
+        if UNCOMMON (index > maxIndex)
   return (NULL);
 
-        return (object->data[index]);
+        return (object->data[index - 1]);
     }
 #undef DO_INDEX_OOP
 
@@ -1117,8 +1117,6 @@ index_oop_put_spec (OOP oop,
   if UNCOMMON (index < 1)
     return (false);
 
-  index--;
-
 #define DO_INDEX_OOP_PUT(type, cond, src) \
     if COMMON (cond) \
       { \
@@ -1132,9 +1130,11 @@ index_oop_put_spec (OOP oop,
           + (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
  \
         /* Check that we're on bounds.  */ \
-        if UNCOMMON (index + sizeof(type) > maxByte) \
+        if UNCOMMON (index > maxByte) \
           return (false); \
  \
+        index -= sizeof(type); \
+ \
         /* Use a cast if unaligned accesses are ok, else memcpy.  */ \
         if (sizeof (type) <= sizeof (PTR)) \
   { \
@@ -1251,10 +1251,10 @@ index_oop_put_spec (OOP oop,
       case GST_ISP_POINTER:
         maxIndex = NUM_WORDS (object);
         index += instanceSpec >> ISP_NUMFIXEDFIELDS;
-        if UNCOMMON (index >= maxIndex)
+        if UNCOMMON (index > maxIndex)
   return (false);
 
-        object->data[index] = value;
+        object->data[index - 1] = value;
         return (true);
     }
 #undef DO_INDEX_OOP_PUT
--
1.8.2.1



_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

libgst:
2013-06-26  Paolo Bonzini  <[hidden email]>

        * libgst/dict.inl: Fix overflow check in index_oop_spec and
        index_oop_put_spec.  This use the trick of converting
        (a < x || a > y) to (a - x > y - x).  Adjusting "index" after
        the check helps because we can compare with "> maxByte" instead
        of ">= maxByte + sizeof(type) - 1".  On the other hand, we
        have to do a somewhat ugly adjust to base.
---
 libgst/ChangeLog |  9 +++++++++
 libgst/dict.inl  | 32 ++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index 893c380..fb45930 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,5 +1,14 @@
 2013-06-26  Paolo Bonzini  <[hidden email]>
 
+ * libgst/dict.inl: Fix overflow check in index_oop_spec and
+ index_oop_put_spec.  This use the trick of converting
+ (a < x || a > y) to (a - x > y - x).  Adjusting "index" after
+ the check helps because we can compare with "> maxByte" instead
+ of ">= maxByte + sizeof(type) - 1".  On the other hand, we
+ have to do a somewhat ugly adjust to base.
+
+2013-06-26  Paolo Bonzini  <[hidden email]>
+
  * libgst/dict.inl: In index_oop_spec and index_oop_put_spec,
  move index decrement after the scaling by sizeof(type) and the
  overflow test. This prepares for fixing the overflow test.
diff --git a/libgst/dict.inl b/libgst/dict.inl
index 3b029b0..528f870 100644
--- a/libgst/dict.inl
+++ b/libgst/dict.inl
@@ -979,7 +979,7 @@ index_oop_spec (OOP oop,
         size_t index,
  intptr_t instanceSpec)
 {
-  size_t maxIndex, maxByte;
+  size_t maxIndex, maxByte, base;
   char *src;
 
   if UNCOMMON (index < 1)
@@ -991,12 +991,12 @@ index_oop_spec (OOP oop,
     if (sizeof (type) <= sizeof (PTR)) \
       maxByte -= (oop->flags & EMPTY_BYTES); \
  \
-    index = \
-      index * sizeof(type) \
-      + (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
+    base = (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
+    index = base + index * sizeof(type); \
  \
     /* Check that we're on bounds.  */ \
-    if UNCOMMON (index > maxByte) \
+    base += sizeof(type); \
+    if UNCOMMON (index - base > maxByte - base) \
       return (NULL); \
  \
     index -= sizeof(type); \
@@ -1084,8 +1084,10 @@ index_oop_spec (OOP oop,
 
       case GST_ISP_POINTER:
         maxIndex = NUM_WORDS (object);
-        index += instanceSpec >> ISP_NUMFIXEDFIELDS;
-        if UNCOMMON (index > maxIndex)
+        base = instanceSpec >> ISP_NUMFIXEDFIELDS;
+        index += base;
+        base++;
+        if UNCOMMON (index - base > maxIndex - base)
   return (NULL);
 
         return (object->data[index - 1]);
@@ -1112,7 +1114,7 @@ index_oop_put_spec (OOP oop,
     OOP value,
     intptr_t instanceSpec)
 {
-  size_t maxIndex;
+  size_t maxIndex, base;
 
   if UNCOMMON (index < 1)
     return (false);
@@ -1125,12 +1127,12 @@ index_oop_put_spec (OOP oop,
         if (sizeof (type) <= sizeof (PTR)) \
           maxByte -= (oop->flags & EMPTY_BYTES); \
  \
-        index = \
-          index * sizeof(type) \
-          + (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
+          base = (instanceSpec >> ISP_NUMFIXEDFIELDS) * sizeof (PTR); \
+          index = base + index * sizeof(type); \
  \
         /* Check that we're on bounds.  */ \
-        if UNCOMMON (index > maxByte) \
+        base += sizeof(type); \
+        if UNCOMMON (index - base > maxByte - base) \
           return (false); \
  \
         index -= sizeof(type); \
@@ -1250,8 +1252,10 @@ index_oop_put_spec (OOP oop,
 
       case GST_ISP_POINTER:
         maxIndex = NUM_WORDS (object);
-        index += instanceSpec >> ISP_NUMFIXEDFIELDS;
-        if UNCOMMON (index > maxIndex)
+        base = instanceSpec >> ISP_NUMFIXEDFIELDS;
+        index += base;
+        base++;
+        if UNCOMMON (index - base > maxIndex - base)
   return (false);
 
         object->data[index - 1] = value;
--
1.8.2.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk