Check for add overflow only once (#467)

aggarg · web-flow · commit 58770b0f5121 · 2022-03-03T15:16:39.000-08:00
Update the size calculations such that we only need to check for add
overflow only once. Also, change the way we detect add overflow so that
we do not need to cause an overflow to detect an overflow.

Signed-off-by: Gaurav Aggarwal &lt;aggarg@amazon.com&gt;
diff --git a/portable/MemMang/heap_2.c b/portable/MemMang/heap_2.c
@@ -62,8 +62,14 @@
 /* Assumes 8bit bytes! */
 #define heapBITS_PER_BYTE           ( ( size_t ) 8 )
 
+/* Max value that fits in a size_t type. */
+#define heapSIZE_MAX                ( ~( ( size_t ) 0 ) )
+
 /* Check if multiplying a and b will result in overflow. */
-#define heapMULTIPLY_WILL_OVERFLOW( a, b, max )    ( ( ( a ) > 0 ) && ( ( b ) > ( ( max ) / ( a ) ) ) )
+#define heapMULTIPLY_WILL_OVERFLOW( a, b )    ( ( ( a ) > 0 ) && ( ( b ) > ( heapSIZE_MAX / ( a ) ) ) )
+
+/* Check if adding a and b will result in overflow. */
+#define heapADD_WILL_OVERFLOW( a, b )         ( ( a ) > ( heapSIZE_MAX - ( b ) ) )
 
 /* MSB of the xBlockSize member of an BlockLink_t structure is used to track
  * the allocation status of a block.  When MSB of the xBlockSize member of
@@ -97,7 +103,7 @@ typedef struct A_BLOCK_LINK
 } BlockLink_t;
 
 
-static const uint16_t heapSTRUCT_SIZE = ( ( sizeof( BlockLink_t ) + ( portBYTE_ALIGNMENT - 1 ) ) & ~portBYTE_ALIGNMENT_MASK );
+static const uint16_t heapSTRUCT_SIZE = ( ( sizeof( BlockLink_t ) + ( portBYTE_ALIGNMENT - 1 ) ) & ~( ( size_t ) portBYTE_ALIGNMENT_MASK ) );
 #define heapMINIMUM_BLOCK_SIZE    ( ( size_t ) ( heapSTRUCT_SIZE * 2 ) )
 
 /* Create a couple of list links to mark the start and end of the list. */
@@ -149,6 +155,7 @@ void * pvPortMalloc( size_t xWantedSize )
     BlockLink_t * pxBlock, * pxPreviousBlock, * pxNewBlockLink;
     PRIVILEGED_DATA static BaseType_t xHeapHasBeenInitialised = pdFALSE;
     void * pvReturn = NULL;
+    size_t xAdditionalRequiredSize;
 
     vTaskSuspendAll();
     {
@@ -160,29 +167,22 @@ void * pvPortMalloc( size_t xWantedSize )
             xHeapHasBeenInitialised = pdTRUE;
         }
 
-        /* The wanted size must be increased so it can contain a BlockLink_t
-         * structure in addition to the requested amount of bytes. */
-        if( ( xWantedSize > 0 ) &&
-            ( ( xWantedSize + heapSTRUCT_SIZE ) > xWantedSize ) ) /* Overflow check. */
+        if( xWantedSize > 0 )
         {
-            xWantedSize += heapSTRUCT_SIZE;
+            /* The wanted size must be increased so it can contain a BlockLink_t
+             * structure in addition to the requested amount of bytes. Some
+             * additional increment may also be needed for alignment. */
+            xAdditionalRequiredSize = heapSTRUCT_SIZE + portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK );
 
-            /* Byte alignment required. Check for overflow. */
-            if( ( xWantedSize + ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) ) )
-                > xWantedSize )
+            if( heapADD_WILL_OVERFLOW( xWantedSize, xAdditionalRequiredSize ) == 0 )
             {
-                xWantedSize += ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) );
-                configASSERT( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) == 0 );
+                xWantedSize += xAdditionalRequiredSize;
             }
             else
             {
                 xWantedSize = 0;
             }
         }
-        else
-        {
-            xWantedSize = 0;
-        }
 
         /* Check the block size we are trying to allocate is not so large that the
          * top bit is set.  The top bit of the block size member of the BlockLink_t
@@ -320,9 +320,8 @@ void * pvPortCalloc( size_t xNum,
                      size_t xSize )
 {
     void * pv = NULL;
-    const size_t xSizeMaxValue = ~( ( size_t ) 0 );
 
-    if( !heapMULTIPLY_WILL_OVERFLOW( xNum, xSize, xSizeMaxValue ) )
+    if( heapMULTIPLY_WILL_OVERFLOW( xNum, xSize ) == 0 )
     {
         pv = pvPortMalloc( xNum * xSize );
 
diff --git a/portable/MemMang/heap_4.c b/portable/MemMang/heap_4.c
@@ -61,8 +61,14 @@
 /* Assumes 8bit bytes! */
 #define heapBITS_PER_BYTE         ( ( size_t ) 8 )
 
+/* Max value that fits in a size_t type. */
+#define heapSIZE_MAX              ( ~( ( size_t ) 0 ) )
+
 /* Check if multiplying a and b will result in overflow. */
-#define heapMULTIPLY_WILL_OVERFLOW( a, b, max )    ( ( ( a ) > 0 ) && ( ( b ) > ( ( max ) / ( a ) ) ) )
+#define heapMULTIPLY_WILL_OVERFLOW( a, b )    ( ( ( a ) > 0 ) && ( ( b ) > ( heapSIZE_MAX / ( a ) ) ) )
+
+/* Check if adding a and b will result in overflow. */
+#define heapADD_WILL_OVERFLOW( a, b )         ( ( a ) > ( heapSIZE_MAX - ( b ) ) )
 
 /* MSB of the xBlockSize member of an BlockLink_t structure is used to track
  * the allocation status of a block.  When MSB of the xBlockSize member of
@@ -132,6 +138,7 @@ void * pvPortMalloc( size_t xWantedSize )
 {
     BlockLink_t * pxBlock, * pxPreviousBlock, * pxNewBlockLink;
     void * pvReturn = NULL;
+    size_t xAdditionalRequiredSize;
 
     vTaskSuspendAll();
     {
@@ -146,35 +153,25 @@ void * pvPortMalloc( size_t xWantedSize )
             mtCOVERAGE_TEST_MARKER();
         }
 
-        /* The wanted size must be increased so it can contain a BlockLink_t
-         * structure in addition to the requested amount of bytes. */
-        if( ( xWantedSize > 0 ) &&
-            ( ( xWantedSize + xHeapStructSize ) > xWantedSize ) ) /* Overflow check. */
+        if( xWantedSize > 0 )
         {
-            xWantedSize += xHeapStructSize;
+            /* The wanted size must be increased so it can contain a BlockLink_t
+             * structure in addition to the requested amount of bytes. Some
+             * additional increment may also be needed for alignment. */
+            xAdditionalRequiredSize = xHeapStructSize + portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK );
 
-            /* Ensure that blocks are always aligned. */
-            if( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) != 0x00 )
+            if( heapADD_WILL_OVERFLOW( xWantedSize, xAdditionalRequiredSize ) == 0 )
             {
-                /* Byte alignment required. Check for overflow. */
-                if( ( xWantedSize + ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) ) ) > xWantedSize )
-                {
-                    xWantedSize += ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) );
-                    configASSERT( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) == 0 );
-                }
-                else
-                {
-                    xWantedSize = 0;
-                }
+                xWantedSize += xAdditionalRequiredSize;
             }
             else
             {
-                mtCOVERAGE_TEST_MARKER();
+                xWantedSize = 0;
             }
         }
         else
         {
-            xWantedSize = 0;
+            mtCOVERAGE_TEST_MARKER();
         }
 
         /* Check the block size we are trying to allocate is not so large that the
@@ -362,9 +359,8 @@ void * pvPortCalloc( size_t xNum,
                      size_t xSize )
 {
     void * pv = NULL;
-    const size_t xSizeMaxValue = ~( ( size_t ) 0 );
 
-    if( !heapMULTIPLY_WILL_OVERFLOW( xNum, xSize, xSizeMaxValue ) )
+    if( heapMULTIPLY_WILL_OVERFLOW( xNum, xSize ) == 0 )
     {
         pv = pvPortMalloc( xNum * xSize );
 
diff --git a/portable/MemMang/heap_5.c b/portable/MemMang/heap_5.c
@@ -95,8 +95,14 @@
 /* Assumes 8bit bytes! */
 #define heapBITS_PER_BYTE         ( ( size_t ) 8 )
 
+/* Max value that fits in a size_t type. */
+#define heapSIZE_MAX              ( ~( ( size_t ) 0 ) )
+
 /* Check if multiplying a and b will result in overflow. */
-#define heapMULTIPLY_WILL_OVERFLOW( a, b, max )    ( ( ( a ) > 0 ) && ( ( b ) > ( ( max ) / ( a ) ) ) )
+#define heapMULTIPLY_WILL_OVERFLOW( a, b )    ( ( ( a ) > 0 ) && ( ( b ) > ( heapSIZE_MAX / ( a ) ) ) )
+
+/* Check if adding a and b will result in overflow. */
+#define heapADD_WILL_OVERFLOW( a, b )         ( ( a ) > ( heapSIZE_MAX - ( b ) ) )
 
 /* MSB of the xBlockSize member of an BlockLink_t structure is used to track
  * the allocation status of a block.  When MSB of the xBlockSize member of
@@ -150,42 +156,33 @@ void * pvPortMalloc( size_t xWantedSize )
 {
     BlockLink_t * pxBlock, * pxPreviousBlock, * pxNewBlockLink;
     void * pvReturn = NULL;
+    size_t xAdditionalRequiredSize;
 
     /* The heap must be initialised before the first call to
      * prvPortMalloc(). */
     configASSERT( pxEnd );
 
     vTaskSuspendAll();
     {
-        /* The wanted size is increased so it can contain a BlockLink_t
-         * structure in addition to the requested amount of bytes. */
-        if( ( xWantedSize > 0 ) &&
-            ( ( xWantedSize + xHeapStructSize ) > xWantedSize ) ) /* Overflow check. */
+        if( xWantedSize > 0 )
         {
-            xWantedSize += xHeapStructSize;
+            /* The wanted size must be increased so it can contain a BlockLink_t
+             * structure in addition to the requested amount of bytes. Some
+             * additional increment may also be needed for alignment. */
+            xAdditionalRequiredSize = xHeapStructSize + portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK );
 
-            /* Ensure that blocks are always aligned */
-            if( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) != 0x00 )
+            if( heapADD_WILL_OVERFLOW( xWantedSize, xAdditionalRequiredSize ) == 0 )
             {
-                /* Byte alignment required. Check for overflow */
-                if( ( xWantedSize + ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) ) ) >
-                    xWantedSize )
-                {
-                    xWantedSize += ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) );
-                }
-                else
-                {
-                    xWantedSize = 0;
-                }
+                xWantedSize += xAdditionalRequiredSize;
             }
             else
             {
-                mtCOVERAGE_TEST_MARKER();
+                xWantedSize = 0;
             }
         }
         else
         {
-            xWantedSize = 0;
+            mtCOVERAGE_TEST_MARKER();
         }
 
         /* Check the block size we are trying to allocate is not so large that the
@@ -365,9 +362,8 @@ void * pvPortCalloc( size_t xNum,
                      size_t xSize )
 {
     void * pv = NULL;
-    const size_t xSizeMaxValue = ~( ( size_t ) 0 );
 
-    if( !heapMULTIPLY_WILL_OVERFLOW( xNum, xSize, xSizeMaxValue ) )
+    if( heapMULTIPLY_WILL_OVERFLOW( xNum, xSize ) == 0 )
     {
         pv = pvPortMalloc( xNum * xSize );
 
@@ -502,7 +498,7 @@ void vPortDefineHeapRegions( const HeapRegion_t * const pxHeapRegions )
          * inserted at the end of the region space. */
         xAddress = xAlignedHeap + xTotalRegionSize;
         xAddress -= xHeapStructSize;
-        xAddress &= ~portBYTE_ALIGNMENT_MASK;
+        xAddress &= ~( ( size_t ) portBYTE_ALIGNMENT_MASK );
         pxEnd = ( BlockLink_t * ) xAddress;
         pxEnd->xBlockSize = 0;
         pxEnd->pxNextFreeBlock = NULL;

Original file line number	Diff line number	Diff line change
`@@ -61,8 +61,14 @@`
`61`	`61`	`/* Assumes 8bit bytes! */`
`62`	`62`	`#define heapBITS_PER_BYTE ( ( size_t ) 8 )`
`63`	`63`
	`64`	`+/* Max value that fits in a size_t type. */`
	`65`	`+#define heapSIZE_MAX ( ~( ( size_t ) 0 ) )`
	`66`	`+`
`64`	`67`	`/* Check if multiplying a and b will result in overflow. */`
`65`		`-#define heapMULTIPLY_WILL_OVERFLOW( a, b, max ) ( ( ( a ) > 0 ) && ( ( b ) > ( ( max ) / ( a ) ) ) )`
	`68`	`+#define heapMULTIPLY_WILL_OVERFLOW( a, b ) ( ( ( a ) > 0 ) && ( ( b ) > ( heapSIZE_MAX / ( a ) ) ) )`
	`69`	`+`
	`70`	`+/* Check if adding a and b will result in overflow. */`
	`71`	`+#define heapADD_WILL_OVERFLOW( a, b ) ( ( a ) > ( heapSIZE_MAX - ( b ) ) )`
`66`	`72`
`67`	`73`	`/* MSB of the xBlockSize member of an BlockLink_t structure is used to track`
`68`	`74`	`* the allocation status of a block. When MSB of the xBlockSize member of`
`@@ -132,6 +138,7 @@ void * pvPortMalloc( size_t xWantedSize )`
`132`	`138`	`{`
`133`	`139`	`BlockLink_t * pxBlock, * pxPreviousBlock, * pxNewBlockLink;`
`134`	`140`	`void * pvReturn = NULL;`
	`141`	`+ size_t xAdditionalRequiredSize;`
`135`	`142`
`136`	`143`	`vTaskSuspendAll();`
`137`	`144`	`{`
`@@ -146,35 +153,25 @@ void * pvPortMalloc( size_t xWantedSize )`
`146`	`153`	`mtCOVERAGE_TEST_MARKER();`
`147`	`154`	`}`
`148`	`155`
`149`		`- /* The wanted size must be increased so it can contain a BlockLink_t`
`150`		`- * structure in addition to the requested amount of bytes. */`
`151`		`- if( ( xWantedSize > 0 ) &&`
`152`		`- ( ( xWantedSize + xHeapStructSize ) > xWantedSize ) ) /* Overflow check. */`
	`156`	`+ if( xWantedSize > 0 )`
`153`	`157`	`{`
`154`		`- xWantedSize += xHeapStructSize;`
	`158`	`+ /* The wanted size must be increased so it can contain a BlockLink_t`
	`159`	`+ * structure in addition to the requested amount of bytes. Some`
	`160`	`+ * additional increment may also be needed for alignment. */`
	`161`	`+ xAdditionalRequiredSize = xHeapStructSize + portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK );`
`155`	`162`
`156`		`- /* Ensure that blocks are always aligned. */`
`157`		`- if( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) != 0x00 )`
	`163`	`+ if( heapADD_WILL_OVERFLOW( xWantedSize, xAdditionalRequiredSize ) == 0 )`
`158`	`164`	`{`
`159`		`- /* Byte alignment required. Check for overflow. */`
`160`		`- if( ( xWantedSize + ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) ) ) > xWantedSize )`
`161`		`- {`
`162`		`- xWantedSize += ( portBYTE_ALIGNMENT - ( xWantedSize & portBYTE_ALIGNMENT_MASK ) );`
`163`		`- configASSERT( ( xWantedSize & portBYTE_ALIGNMENT_MASK ) == 0 );`
`164`		`- }`
`165`		`- else`
`166`		`- {`
`167`		`- xWantedSize = 0;`
`168`		`- }`
	`165`	`+ xWantedSize += xAdditionalRequiredSize;`
`169`	`166`	`}`
`170`	`167`	`else`
`171`	`168`	`{`
`172`		`- mtCOVERAGE_TEST_MARKER();`
	`169`	`+ xWantedSize = 0;`
`173`	`170`	`}`
`174`	`171`	`}`
`175`	`172`	`else`
`176`	`173`	`{`
`177`		`- xWantedSize = 0;`
	`174`	`+ mtCOVERAGE_TEST_MARKER();`
`178`	`175`	`}`
`179`	`176`
`180`	`177`	`/* Check the block size we are trying to allocate is not so large that the`
`@@ -362,9 +359,8 @@ void * pvPortCalloc( size_t xNum,`
`362`	`359`	`size_t xSize )`
`363`	`360`	`{`
`364`	`361`	`void * pv = NULL;`
`365`		`- const size_t xSizeMaxValue = ~( ( size_t ) 0 );`
`366`	`362`
`367`		`- if( !heapMULTIPLY_WILL_OVERFLOW( xNum, xSize, xSizeMaxValue ) )`
	`363`	`+ if( heapMULTIPLY_WILL_OVERFLOW( xNum, xSize ) == 0 )`
`368`	`364`	`{`
`369`	`365`	`pv = pvPortMalloc( xNum * xSize );`
`370`	`366`