Merge pull request #1413 from felixhandte/attach-dict-fix-unsigned-compare

Fix #1412: Perform Signed Comparison When Setting Attach Dict Param

lib/compress/zstd_compress.c

@@ -412,11 +412,12 @@ size_t ZSTD_CCtxParam_setParameter(
         CCtxParams->forceWindow = (value > 0);
         return CCtxParams->forceWindow;
 
-    case ZSTD_p_forceAttachDict :
+    case ZSTD_p_forceAttachDict : {
-        CCtxParams->attachDictPref = value ?
+        const ZSTD_dictAttachPref_e pref = (ZSTD_dictAttachPref_e)value;
-                                    (value > 0 ? ZSTD_dictForceAttach : ZSTD_dictForceCopy) :
+        CLAMPCHECK(pref, ZSTD_dictDefaultAttach, ZSTD_dictForceCopy);
-                                     ZSTD_dictDefaultAttach;
+        CCtxParams->attachDictPref = pref;
         return CCtxParams->attachDictPref;
+    }
 
     case ZSTD_p_nbWorkers :
 #ifndef ZSTD_MULTITHREAD

lib/compress/zstd_compress_internal.h

@@ -48,12 +48,6 @@ extern "C" {
 typedef enum { ZSTDcs_created=0, ZSTDcs_init, ZSTDcs_ongoing, ZSTDcs_ending } ZSTD_compressionStage_e;
 typedef enum { zcss_init=0, zcss_load, zcss_flush } ZSTD_cStreamStage;
 
-typedef enum {
-    ZSTD_dictDefaultAttach = 0,
-    ZSTD_dictForceAttach = 1,
-    ZSTD_dictForceCopy = -1,
-} ZSTD_dictAttachPref_e;
-
 typedef struct ZSTD_prefixDict_s {
     const void* dict;
     size_t dictSize;

lib/zstd.h

@@ -996,6 +996,38 @@ typedef enum {
                               * Decoder cannot recognise automatically this format, requiring instructions. */
 } ZSTD_format_e;
 
+typedef enum {
+    /* Note: this enum and the behavior it controls are effectively internal
+     * implementation details of the compressor. They are expected to continue
+     * to evolve and should be considered only in the context of extremely
+     * advanced performance tuning.
+     *
+     * Zstd currently supports the use of a CDict in two ways:
+     *
+     * - The contents of the CDict can be copied into the working context. This
+     *   means that the compression can search both the dictionary and input
+     *   while operating on a single set of internal tables. This makes
+     *   the compression faster per-byte of input. However, the initial copy of
+     *   the CDict's tables incurs a fixed cost at the beginning of the
+     *   compression. For small compressions (< 8 KB), that copy can dominate
+     *   the cost of the compression.
+     *
+     * - The CDict's tables can be used in-place. In this model, compression is
+     *   slower per input byte, because the compressor has to search two sets of
+     *   tables. However, this model incurs no start-up cost (as long as the
+     *   working context's tables can be reused). For small inputs, this can be
+     *   faster than copying the CDict's tables.
+     *
+     * Zstd has a simple internal heuristic that selects which strategy to use
+     * at the beginning of a compression. However, if experimentation shows that
+     * Zstd is making poor choices, it is possible to override that choice with
+     * this enum.
+     */
+    ZSTD_dictDefaultAttach = 0, /* Use the default heuristic. */
+    ZSTD_dictForceAttach   = 1, /* Never copy the dictionary. */
+    ZSTD_dictForceCopy     = 2, /* Always copy the dictionary. */
+} ZSTD_dictAttachPref_e;
+
 typedef enum {
     /* compression format */
     ZSTD_p_format = 10,      /* See ZSTD_format_e enum definition.
@@ -1109,29 +1141,14 @@ typedef enum {
 
     ZSTD_p_forceMaxWindow=1100, /* Force back-reference distances to remain < windowSize,
                               * even when referencing into Dictionary content (default:0) */
-    ZSTD_p_forceAttachDict,  /* ZSTD supports usage of a CDict in-place
+    ZSTD_p_forceAttachDict,  /* Controls whether the contents of a CDict are
-                              * (avoiding having to copy the compression tables
+                              * used in place, or whether they are copied into
-                              * from the CDict into the working context). Using
+                              * the working context.
-                              * a CDict in this way saves an initial setup step,
-                              * but comes at the cost of more work per byte of
-                              * input. ZSTD has a simple internal heuristic that
-                              * guesses which strategy will be faster. You can
-                              * use this flag to override that guess.
                               *
-                              * Note that the by-reference, in-place strategy is
+                              * Accepts values from the ZSTD_dictAttachPref_e
-                              * only used when reusing a compression context
+                              * enum. See the comments on that enum for an
-                              * with compatible compression parameters. (If
+                              * explanation of the feature.
-                              * incompatible / uninitialized, the working
-                              * context needs to be cleared anyways, which is
-                              * about as expensive as overwriting it with the
-                              * dictionary context, so there's no savings in
-                              * using the CDict by-ref.)
-                              *
-                              * Values greater than 0 force attaching the dict.
-                              * Values less than 0 force copying the dict.
-                              * 0 selects the default heuristic-guided behavior.
                               */
-
 } ZSTD_cParameter;
 
 

tests/fuzz/zstd_helpers.c

@@ -71,7 +71,7 @@ void FUZZ_setRandomParameters(ZSTD_CCtx *cctx, size_t srcSize, uint32_t *state)
     setRand(cctx, ZSTD_p_contentSizeFlag, 0, 1, state);
     setRand(cctx, ZSTD_p_checksumFlag, 0, 1, state);
     setRand(cctx, ZSTD_p_dictIDFlag, 0, 1, state);
-    setRand(cctx, ZSTD_p_forceAttachDict, -2, 2, state);
+    setRand(cctx, ZSTD_p_forceAttachDict, 0, 2, state);
     /* Select long distance matchig parameters */
     setRand(cctx, ZSTD_p_enableLongDistanceMatching, 0, 1, state);
     setRand(cctx, ZSTD_p_ldmHashLog, ZSTD_HASHLOG_MIN, 16, state);