cover: reject unrepresentable dmer map sizes in COVER_map_init

lib/dictBuilder/cover.c

@@ -154,15 +154,37 @@ static void COVER_map_clear(COVER_map_t *map) {
  * The map is only guaranteed to be large enough to hold size elements.
  */
 static int COVER_map_init(COVER_map_t *map, U32 size) {
-  map->sizeLog = ZSTD_highbit32(size) + 2;
-  map->size = (U32)1 << map->sizeLog;
-  map->sizeMask = map->size - 1;
-  map->data = (COVER_map_pair_t *)malloc(map->size * sizeof(COVER_map_pair_t));
-  if (!map->data) {
-    map->sizeLog = 0;
-    map->size = 0;
-    return 0;
-  }
+  U32 sizeLog;
+  U32 mapSize;
+  size_t allocSize;
+
+  map->sizeLog = 0;
+  map->size = 0;
+  map->sizeMask = 0;
+  map->data = NULL;
+
+  /* The capacity is size rounded up to the next power of two, stored in a
+   * U32. Reject sizes that cannot be represented instead of computing with
+   * undefined behaviour and under-allocating:
+   * - size == 0 would make ZSTD_highbit32() undefined;
+   * - size >= 2^30 gives sizeLog >= 32, i.e. a shift of 32 or more in
+   *   `(U32)1 << sizeLog`, which is undefined for a 32-bit type;
+   * - the byte size of the pair array must not wrap size_t (it does for
+   *   mapSize > SIZE_MAX / sizeof(COVER_map_pair_t) on 32-bit targets).
+   */
+  if (size == 0) return 0;
+  sizeLog = ZSTD_highbit32(size) + 2;
+  if (sizeLog >= 32) return 0;
+  mapSize = (U32)1U << sizeLog;
+  allocSize = (size_t)mapSize * sizeof(COVER_map_pair_t);
+  if (allocSize / sizeof(COVER_map_pair_t) != (size_t)mapSize) return 0;
+
+  map->data = (COVER_map_pair_t *)malloc(allocSize);
+  if (!map->data) return 0;
+
+  map->sizeLog = sizeLog;
+  map->size = mapSize;
+  map->sizeMask = mapSize - 1;
   COVER_map_clear(map);
   return 1;
 }
@@ -586,6 +608,11 @@ static int COVER_checkParameters(ZDICT_cover_params_t parameters,
   if (parameters.d > parameters.k) {
     return 0;
   }
+  /* k-d+1 is the number of dmers to track in COVER_map_init(); its capacity
+   * is rounded up to a power of two in a U32, so it must stay below 2^30 */
+  if (parameters.k - parameters.d + 1 >= ((U32)1 << 30)) {
+    return 0;
+  }
   /* 0 < splitPoint <= 1 */
   if (parameters.splitPoint <= 0 || parameters.splitPoint > 1){
     return 0;

tests/fuzzer.c

@@ -26,6 +26,10 @@
 #include <stdio.h>        /* fgets, sscanf */
 #include <string.h>       /* strcmp */
 #include <time.h>         /* time(), time_t */
+#ifndef _WIN32
+#  include <unistd.h>     /* fork, _exit */
+#  include <sys/wait.h>   /* waitpid */
+#endif
 #undef NDEBUG             /* always enable assert() */
 #include <assert.h>
 #define ZSTD_STATIC_LINKING_ONLY  /* ZSTD_compressContinue, ZSTD_compressBlock */
@@ -3120,6 +3124,55 @@ static int basicUnitTests(U32 const seed, double compressibility)
         }
         DISPLAYLEVEL(3, "OK, created dictionary of size %u \n", (unsigned)dictSize);
 
+        DISPLAYLEVEL(3, "test%3i : COVER accepts ordinary k/d : ", testNb++);
+        {   ZDICT_cover_params_t coverParams;
+            memset(&coverParams, 0, sizeof(coverParams));
+            coverParams.k = 64;
+            coverParams.d = 8;
+            coverParams.steps = 1;
+            coverParams.splitPoint = 0.75;
+            dictSize = ZDICT_trainFromBuffer_cover(dictBuffer, dictBufferCapacity,
+                                                   CNBuffer, samplesSizes, nbSamples,
+                                                   coverParams);
+            if (ZDICT_isError(dictSize)) goto _output_error;
+        }
+        DISPLAYLEVEL(3, "OK, created dictionary of size %u \n", (unsigned)dictSize);
+
+        DISPLAYLEVEL(3, "test%3i : COVER rejects unrepresentable k-d+1 : ", testNb++);
+        {   /* k-d+1 == 2^30 rounds the dmer map capacity up to 2^32 : the
+             * old COVER_map_init() shifted a U32 by 32 (undefined behaviour,
+             * SIGSEGV on common builds) and on 32-bit wrapped the pair-array
+             * allocation to zero. The call must fail cleanly instead. */
+            ZDICT_cover_params_t coverParams;
+            memset(&coverParams, 0, sizeof(coverParams));
+            coverParams.k = (U32)1 << 30;
+            coverParams.d = 1;
+            coverParams.steps = 1;
+            coverParams.splitPoint = 0.75;
+#ifdef _WIN32
+            {   size_t const badSize = ZDICT_trainFromBuffer_cover(
+                    dictBuffer, (size_t)1 << 30,
+                    CNBuffer, samplesSizes, nbSamples, coverParams);
+                if (!ZDICT_isError(badSize)) goto _output_error;
+            }
+#else
+            {   /* crash-isolated : an unfixed tree dies inside this call */
+                pid_t const child = fork();
+                assert(child >= 0);
+                if (child == 0) {
+                    size_t const badSize = ZDICT_trainFromBuffer_cover(
+                        dictBuffer, (size_t)1 << 30,
+                        CNBuffer, samplesSizes, nbSamples, coverParams);
+                    _exit(ZDICT_isError(badSize) ? 0 : 1);
+                }
+                {   int status = 0;
+                    if (waitpid(child, &status, 0) != child) goto _output_error;
+                    if (!WIFEXITED(status) || (WEXITSTATUS(status) != 0)) goto _output_error;
+            }   }
+#endif
+        }
+        DISPLAYLEVEL(3, "OK \n");
+
         DISPLAYLEVEL(3, "test%3i : check dictID : ", testNb++);
         dictID = ZDICT_getDictID(dictBuffer, dictSize);
         if (dictID==0) goto _output_error;