gh-132983: Split `_zstd_set_c_parameters` (#133921)

Lib/compression/zstd/_zstdfile.py

@@ -1,7 +1,6 @@
 import io
 from os import PathLike
-from _zstd import (ZstdCompressor, ZstdDecompressor, ZstdError,
+from _zstd import ZstdCompressor, ZstdDecompressor, ZSTD_DStreamOutSize
-                   ZSTD_DStreamOutSize)
 from compression._common import _streams
 
 __all__ = ('ZstdFile', 'open')

Lib/test/test_zstd.py

@@ -64,6 +64,10 @@
 
 SUPPORT_MULTITHREADING = False
 
+C_INT_MIN = -(2**31)
+C_INT_MAX = (2**31) - 1
+
+
 def setUpModule():
     global SUPPORT_MULTITHREADING
     SUPPORT_MULTITHREADING = CompressionParameter.nb_workers.bounds() != (0, 0)
@@ -195,14 +199,21 @@ def test_simple_compress_bad_args(self):
         self.assertRaises(TypeError, ZstdCompressor, zstd_dict=b"abcd1234")
         self.assertRaises(TypeError, ZstdCompressor, zstd_dict={1: 2, 3: 4})
 
-        with self.assertRaises(ValueError):
+        # valid range for compression level is [-(1<<17), 22]
-            ZstdCompressor(2**31)
+        msg = r'illegal compression level {}; the valid range is \[-?\d+, -?\d+\]'
-        with self.assertRaises(ValueError):
+        with self.assertRaisesRegex(ValueError, msg.format(C_INT_MAX)):
-            ZstdCompressor(options={2**31: 100})
+            ZstdCompressor(C_INT_MAX)
+        with self.assertRaisesRegex(ValueError, msg.format(C_INT_MIN)):
+            ZstdCompressor(C_INT_MIN)
+        msg = r'illegal compression level; the valid range is \[-?\d+, -?\d+\]'
+        with self.assertRaisesRegex(ValueError, msg):
+            ZstdCompressor(level=-(2**1000))
+        with self.assertRaisesRegex(ValueError, msg):
+            ZstdCompressor(level=2**1000)
 
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdCompressor(options={CompressionParameter.window_log: 100})
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdCompressor(options={3333: 100})
 
         # Method bad arguments
@@ -253,18 +264,32 @@ def test_compress_parameters(self):
              }
         ZstdCompressor(options=d)
 
-        # larger than signed int, ValueError
         d1 = d.copy()
-        d1[CompressionParameter.ldm_bucket_size_log] = 2**31
+        # larger than signed int
-        self.assertRaises(ValueError, ZstdCompressor, options=d1)
+        d1[CompressionParameter.ldm_bucket_size_log] = C_INT_MAX
+        with self.assertRaises(ValueError):
+            ZstdCompressor(options=d1)
+        # smaller than signed int
+        d1[CompressionParameter.ldm_bucket_size_log] = C_INT_MIN
+        with self.assertRaises(ValueError):
+            ZstdCompressor(options=d1)
 
-        # clamp compressionLevel
+        # out of bounds compression level
         level_min, level_max = CompressionParameter.compression_level.bounds()
+        with self.assertRaises(ValueError):
             compress(b'', level_max+1)
+        with self.assertRaises(ValueError):
             compress(b'', level_min-1)
-
+        with self.assertRaises(ValueError):
-        compress(b'', options={CompressionParameter.compression_level:level_max+1})
+            compress(b'', 2**1000)
-        compress(b'', options={CompressionParameter.compression_level:level_min-1})
+        with self.assertRaises(ValueError):
+            compress(b'', -(2**1000))
+        with self.assertRaises(ValueError):
+            compress(b'', options={
+                CompressionParameter.compression_level: level_max+1})
+        with self.assertRaises(ValueError):
+            compress(b'', options={
+                CompressionParameter.compression_level: level_min-1})
 
         # zstd lib doesn't support MT compression
         if not SUPPORT_MULTITHREADING:
@@ -277,19 +302,19 @@ def test_compress_parameters(self):
 
         # out of bounds error msg
         option = {CompressionParameter.window_log:100}
-        with self.assertRaisesRegex(ZstdError,
+        with self.assertRaisesRegex(
-                (r'Error when setting zstd compression parameter "window_log", '
+            ValueError,
-                 r'it should \d+ <= value <= \d+, provided value is 100\. '
+            "compression parameter 'window_log' received an illegal value 100; "
-                 r'\((?:32|64)-bit build\)')):
+            r'the valid range is \[-?\d+, -?\d+\]',
+        ):
             compress(b'', options=option)
 
     def test_unknown_compression_parameter(self):
         KEY = 100001234
         option = {CompressionParameter.compression_level: 10,
                   KEY: 200000000}
-        pattern = (r'Invalid zstd compression parameter.*?'
+        pattern = rf"invalid compression parameter 'unknown parameter \(key {KEY}\)'"
-                   fr'"unknown parameter \(key {KEY}\)"')
+        with self.assertRaisesRegex(ValueError, pattern):
-        with self.assertRaisesRegex(ZstdError, pattern):
             ZstdCompressor(options=option)
 
     @unittest.skipIf(not SUPPORT_MULTITHREADING,
@@ -384,11 +409,21 @@ def test_simple_decompress_bad_args(self):
         self.assertRaises(TypeError, ZstdDecompressor, options=b'abc')
 
         with self.assertRaises(ValueError):
-            ZstdDecompressor(options={2**31 : 100})
+            ZstdDecompressor(options={C_INT_MAX: 100})
+        with self.assertRaises(ValueError):
+            ZstdDecompressor(options={C_INT_MIN: 100})
+        with self.assertRaises(ValueError):
+            ZstdDecompressor(options={0: C_INT_MAX})
+        with self.assertRaises(OverflowError):
+            ZstdDecompressor(options={2**1000: 100})
+        with self.assertRaises(OverflowError):
+            ZstdDecompressor(options={-(2**1000): 100})
+        with self.assertRaises(OverflowError):
+            ZstdDecompressor(options={0: -(2**1000)})
 
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdDecompressor(options={DecompressionParameter.window_log_max: 100})
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdDecompressor(options={3333: 100})
 
         empty = compress(b'')
@@ -402,26 +437,52 @@ def test_decompress_parameters(self):
         d = {DecompressionParameter.window_log_max : 15}
         ZstdDecompressor(options=d)
 
-        # larger than signed int, ValueError
         d1 = d.copy()
-        d1[DecompressionParameter.window_log_max] = 2**31
+        # larger than signed int
-        self.assertRaises(ValueError, ZstdDecompressor, None, d1)
+        d1[DecompressionParameter.window_log_max] = 2**1000
+        with self.assertRaises(OverflowError):
+            ZstdDecompressor(None, d1)
+        # smaller than signed int
+        d1[DecompressionParameter.window_log_max] = -(2**1000)
+        with self.assertRaises(OverflowError):
+            ZstdDecompressor(None, d1)
+
+        d1[DecompressionParameter.window_log_max] = C_INT_MAX
+        with self.assertRaises(ValueError):
+            ZstdDecompressor(None, d1)
+        d1[DecompressionParameter.window_log_max] = C_INT_MIN
+        with self.assertRaises(ValueError):
+            ZstdDecompressor(None, d1)
 
         # out of bounds error msg
         options = {DecompressionParameter.window_log_max:100}
-        with self.assertRaisesRegex(ZstdError,
+        with self.assertRaisesRegex(
-                (r'Error when setting zstd decompression parameter "window_log_max", '
+            ValueError,
-                 r'it should \d+ <= value <= \d+, provided value is 100\. '
+            "decompression parameter 'window_log_max' received an illegal value 100; "
-                 r'\((?:32|64)-bit build\)')):
+            r'the valid range is \[-?\d+, -?\d+\]',
+        ):
+            decompress(b'', options=options)
+
+        # out of bounds deecompression parameter
+        options[DecompressionParameter.window_log_max] = C_INT_MAX
+        with self.assertRaises(ValueError):
+            decompress(b'', options=options)
+        options[DecompressionParameter.window_log_max] = C_INT_MIN
+        with self.assertRaises(ValueError):
+            decompress(b'', options=options)
+        options[DecompressionParameter.window_log_max] = 2**1000
+        with self.assertRaises(OverflowError):
+            decompress(b'', options=options)
+        options[DecompressionParameter.window_log_max] = -(2**1000)
+        with self.assertRaises(OverflowError):
             decompress(b'', options=options)
 
     def test_unknown_decompression_parameter(self):
         KEY = 100001234
         options = {DecompressionParameter.window_log_max: DecompressionParameter.window_log_max.bounds()[1],
                   KEY: 200000000}
-        pattern = (r'Invalid zstd decompression parameter.*?'
+        pattern = rf"invalid decompression parameter 'unknown parameter \(key {KEY}\)'"
-                   fr'"unknown parameter \(key {KEY}\)"')
+        with self.assertRaisesRegex(ValueError, pattern):
-        with self.assertRaisesRegex(ZstdError, pattern):
             ZstdDecompressor(options=options)
 
     def test_decompress_epilogue_flags(self):
@@ -1424,11 +1485,11 @@ def test_init_bad_mode(self):
             ZstdFile(io.BytesIO(COMPRESSED_100_PLUS_32KB), "rw")
 
         with self.assertRaisesRegex(TypeError,
-                                    r"NOT be a CompressionParameter"):
+                                    r"not be a CompressionParameter"):
             ZstdFile(io.BytesIO(), 'rb',
                      options={CompressionParameter.compression_level:5})
         with self.assertRaisesRegex(TypeError,
-                                    r"NOT be a DecompressionParameter"):
+                                    r"not be a DecompressionParameter"):
             ZstdFile(io.BytesIO(), 'wb',
                      options={DecompressionParameter.window_log_max:21})
 
@@ -1439,19 +1500,19 @@ def test_init_bad_check(self):
         with self.assertRaises(TypeError):
             ZstdFile(io.BytesIO(), "w", level='asd')
         # CHECK_UNKNOWN and anything above CHECK_ID_MAX should be invalid.
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdFile(io.BytesIO(), "w", options={999:9999})
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdFile(io.BytesIO(), "w", options={CompressionParameter.window_log:99})
 
         with self.assertRaises(TypeError):
             ZstdFile(io.BytesIO(COMPRESSED_100_PLUS_32KB), "r", options=33)
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(OverflowError):
             ZstdFile(io.BytesIO(COMPRESSED_100_PLUS_32KB),
                              options={DecompressionParameter.window_log_max:2**31})
 
-        with self.assertRaises(ZstdError):
+        with self.assertRaises(ValueError):
             ZstdFile(io.BytesIO(COMPRESSED_100_PLUS_32KB),
                              options={444:333})
 
@@ -1467,7 +1528,7 @@ def test_init_close_fp(self):
             tmp_f.write(DAT_130K_C)
             filename = tmp_f.name
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(TypeError):
             ZstdFile(filename, options={'a':'b'})
 
         # for PyPy

Modules/_zstd/_zstdmodule.c

@@ -103,16 +103,13 @@ static const ParameterInfo dp_list[] = {
 };
 
 void
-set_parameter_error(const _zstd_state* const state, int is_compress,
+set_parameter_error(int is_compress, int key_v, int value_v)
-                    int key_v, int value_v)
 {
     ParameterInfo const *list;
     int list_size;
-    char const *name;
     char *type;
     ZSTD_bounds bounds;
-    int i;
+    char pos_msg[64];
-    char pos_msg[128];
 
     if (is_compress) {
         list = cp_list;
@@ -126,8 +123,8 @@ set_parameter_error(const _zstd_state* const state, int is_compress,
     }
 
     /* Find parameter's name */
-    name = NULL;
+    char const *name = NULL;
-    for (i = 0; i < list_size; i++) {
+    for (int i = 0; i < list_size; i++) {
         if (key_v == (list+i)->parameter) {
             name = (list+i)->parameter_name;
             break;
@@ -149,20 +146,16 @@ set_parameter_error(const _zstd_state* const state, int is_compress,
         bounds = ZSTD_dParam_getBounds(key_v);
     }
     if (ZSTD_isError(bounds.error)) {
-        PyErr_Format(state->ZstdError,
+        PyErr_Format(PyExc_ValueError, "invalid %s parameter '%s'",
-                     "Invalid zstd %s parameter \"%s\".",
                      type, name);
         return;
     }
 
     /* Error message */
-    PyErr_Format(state->ZstdError,
+    PyErr_Format(PyExc_ValueError,
-                 "Error when setting zstd %s parameter \"%s\", it "
+        "%s parameter '%s' received an illegal value %d; "
-                 "should %d <= value <= %d, provided value is %d. "
+        "the valid range is [%d, %d]",
-                 "(%d-bit build)",
+        type, name, value_v, bounds.lowerBound, bounds.upperBound);
-                 type, name,
-                 bounds.lowerBound, bounds.upperBound, value_v,
-                 8*(int)sizeof(Py_ssize_t));
 }
 
 static inline _zstd_state*

Modules/_zstd/_zstdmodule.h

@@ -49,7 +49,6 @@ set_zstd_error(const _zstd_state* const state,
                const error_type type, size_t zstd_ret);
 
 extern void
-set_parameter_error(const _zstd_state* const state, int is_compress,
+set_parameter_error(int is_compress, int key_v, int value_v);
-                    int key_v, int value_v);
 
 #endif  // !ZSTD_MODULE_H

Modules/_zstd/compressor.c

@@ -49,22 +49,15 @@ typedef struct {
 #include "clinic/compressor.c.h"
 
 static int
-_zstd_set_c_parameters(ZstdCompressor *self, PyObject *level_or_options,
+_zstd_set_c_level(ZstdCompressor *self, int level)
-                       const char *arg_name, const char* arg_type)
 {
-    size_t zstd_ret;
+    /* Set integer compression level */
-    _zstd_state* mod_state = PyType_GetModuleState(Py_TYPE(self));
+    int min_level = ZSTD_minCLevel();
-    if (mod_state == NULL) {
+    int max_level = ZSTD_maxCLevel();
-        return -1;
+    if (level < min_level || level > max_level) {
-    }
-
-    /* Integer compression level */
-    if (PyLong_Check(level_or_options)) {
-        int level = PyLong_AsInt(level_or_options);
-        if (level == -1 && PyErr_Occurred()) {
         PyErr_Format(PyExc_ValueError,
-                         "Compression level should be an int value between "
+        "illegal compression level %d; the valid range is [%d, %d]",
-                         "%d and %d.", ZSTD_minCLevel(), ZSTD_maxCLevel());
+            level, min_level, max_level);
         return -1;
     }
 
@@ -72,52 +65,69 @@ _zstd_set_c_parameters(ZstdCompressor *self, PyObject *level_or_options,
     self->compression_level = level;
 
     /* Set compressionLevel to compression context */
-        zstd_ret = ZSTD_CCtx_setParameter(self->cctx,
+    size_t zstd_ret = ZSTD_CCtx_setParameter(
-                                          ZSTD_c_compressionLevel,
+        self->cctx, ZSTD_c_compressionLevel, level);
-                                          level);
 
     /* Check error */
     if (ZSTD_isError(zstd_ret)) {
+        _zstd_state* mod_state = PyType_GetModuleState(Py_TYPE(self));
+        if (mod_state == NULL) {
+            return -1;
+        }
         set_zstd_error(mod_state, ERR_SET_C_LEVEL, zstd_ret);
         return -1;
     }
     return 0;
 }
 
-    /* Options dict */
+static int
-    if (PyDict_Check(level_or_options)) {
+_zstd_set_c_parameters(ZstdCompressor *self, PyObject *options)
-        PyObject *key, *value;
+{
-        Py_ssize_t pos = 0;
+    _zstd_state* mod_state = PyType_GetModuleState(Py_TYPE(self));
-
+    if (mod_state == NULL) {
-        while (PyDict_Next(level_or_options, &pos, &key, &value)) {
-            /* Check key type */
-            if (Py_TYPE(key) == mod_state->DParameter_type) {
-                PyErr_SetString(PyExc_TypeError,
-                                "Key of compression options dict should "
-                                "NOT be a DecompressionParameter attribute.");
         return -1;
     }
 
+    if (!PyDict_Check(options)) {
+        PyErr_Format(PyExc_TypeError,
+             "ZstdCompressor() argument 'options' must be dict, not %T",
+             options);
+        return -1;
+    }
+
+    Py_ssize_t pos = 0;
+    PyObject *key, *value;
+    while (PyDict_Next(options, &pos, &key, &value)) {
+        /* Check key type */
+        if (Py_TYPE(key) == mod_state->DParameter_type) {
+            PyErr_SetString(PyExc_TypeError,
+                "compression options dictionary key must not be a "
+                "DecompressionParameter attribute");
+            return -1;
+        }
+
+        Py_INCREF(key);
+        Py_INCREF(value);
         int key_v = PyLong_AsInt(key);
+        Py_DECREF(key);
         if (key_v == -1 && PyErr_Occurred()) {
-                PyErr_SetString(PyExc_ValueError,
+            Py_DECREF(value);
-                                "Key of options dict should be either a "
-                                "CompressionParameter attribute or an int.");
             return -1;
         }
 
         int value_v = PyLong_AsInt(value);
+        Py_DECREF(value);
         if (value_v == -1 && PyErr_Occurred()) {
-                PyErr_SetString(PyExc_ValueError,
-                                "Value of options dict should be an int.");
             return -1;
         }
 
         if (key_v == ZSTD_c_compressionLevel) {
-                /* Save for generating ZSTD_CDICT */
+            if (_zstd_set_c_level(self, value_v) < 0) {
-                self->compression_level = value_v;
+                return -1;
             }
-            else if (key_v == ZSTD_c_nbWorkers) {
+            continue;
+        }
+        if (key_v == ZSTD_c_nbWorkers) {
             /* From the zstd library docs:
                1. When nbWorkers >= 1, triggers asynchronous mode when
                   used with ZSTD_compressStream2().
@@ -130,18 +140,16 @@ _zstd_set_c_parameters(ZstdCompressor *self, PyObject *level_or_options,
         }
 
         /* Set parameter to compression context */
-            zstd_ret = ZSTD_CCtx_setParameter(self->cctx, key_v, value_v);
+        size_t zstd_ret = ZSTD_CCtx_setParameter(self->cctx, key_v, value_v);
+
+        /* Check error */
         if (ZSTD_isError(zstd_ret)) {
-                set_parameter_error(mod_state, 1, key_v, value_v);
+            set_parameter_error(1, key_v, value_v);
             return -1;
         }
     }
     return 0;
 }
-    PyErr_Format(PyExc_TypeError,
-                 "Invalid type for %s. Expected %s", arg_name, arg_type);
-    return -1;
-}
 
 static void
 capsule_free_cdict(PyObject *capsule)
@@ -354,20 +362,35 @@ _zstd_ZstdCompressor_new_impl(PyTypeObject *type, PyObject *level,
     self->last_mode = ZSTD_e_end;
 
     if (level != Py_None && options != Py_None) {
-        PyErr_SetString(PyExc_RuntimeError,
+        PyErr_SetString(PyExc_TypeError,
                         "Only one of level or options should be used.");
         goto error;
     }
 
-    /* Set compressLevel/options to compression context */
+    /* Set compression level */
     if (level != Py_None) {
-        if (_zstd_set_c_parameters(self, level, "level", "int") < 0) {
+        if (!PyLong_Check(level)) {
+            PyErr_SetString(PyExc_TypeError,
+                "invalid type for level, expected int");
+            goto error;
+        }
+        int level_v = PyLong_AsInt(level);
+        if (level_v == -1 && PyErr_Occurred()) {
+            if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
+                PyErr_Format(PyExc_ValueError,
+                    "illegal compression level; the valid range is [%d, %d]",
+                    ZSTD_minCLevel(), ZSTD_maxCLevel());
+            }
+            goto error;
+        }
+        if (_zstd_set_c_level(self, level_v) < 0) {
             goto error;
         }
     }
 
+    /* Set options dictionary */
     if (options != Py_None) {
-        if (_zstd_set_c_parameters(self, options, "options", "dict") < 0) {
+        if (_zstd_set_c_parameters(self, options) < 0) {
             goto error;
         }
     }

Modules/_zstd/decompressor.c

@@ -86,56 +86,52 @@ _get_DDict(ZstdDict *self)
     return self->d_dict;
 }
 
-/* Set decompression parameters to decompression context */
 static int
 _zstd_set_d_parameters(ZstdDecompressor *self, PyObject *options)
 {
-    size_t zstd_ret;
-    PyObject *key, *value;
-    Py_ssize_t pos;
     _zstd_state* mod_state = PyType_GetModuleState(Py_TYPE(self));
     if (mod_state == NULL) {
         return -1;
     }
 
     if (!PyDict_Check(options)) {
-        PyErr_SetString(PyExc_TypeError,
+        PyErr_Format(PyExc_TypeError,
-                        "options argument should be dict object.");
+             "ZstdDecompressor() argument 'options' must be dict, not %T",
+             options);
         return -1;
     }
 
-    pos = 0;
+    Py_ssize_t pos = 0;
+    PyObject *key, *value;
     while (PyDict_Next(options, &pos, &key, &value)) {
         /* Check key type */
         if (Py_TYPE(key) == mod_state->CParameter_type) {
             PyErr_SetString(PyExc_TypeError,
-                            "Key of decompression options dict should "
+                "compression options dictionary key must not be a "
-                            "NOT be a CompressionParameter attribute.");
+                "CompressionParameter attribute");
             return -1;
         }
 
-        /* Both key & value should be 32-bit signed int */
+        Py_INCREF(key);
+        Py_INCREF(value);
         int key_v = PyLong_AsInt(key);
+        Py_DECREF(key);
         if (key_v == -1 && PyErr_Occurred()) {
-            PyErr_SetString(PyExc_ValueError,
-                            "Key of options dict should be either a "
-                            "DecompressionParameter attribute or an int.");
             return -1;
         }
 
         int value_v = PyLong_AsInt(value);
+        Py_DECREF(value);
         if (value_v == -1 && PyErr_Occurred()) {
-            PyErr_SetString(PyExc_ValueError,
-                            "Value of options dict should be an int.");
             return -1;
         }
 
         /* Set parameter to compression context */
-        zstd_ret = ZSTD_DCtx_setParameter(self->dctx, key_v, value_v);
+        size_t zstd_ret = ZSTD_DCtx_setParameter(self->dctx, key_v, value_v);
 
         /* Check error */
         if (ZSTD_isError(zstd_ret)) {
-            set_parameter_error(mod_state, 0, key_v, value_v);
+            set_parameter_error(0, key_v, value_v);
             return -1;
         }
     }
@@ -577,7 +573,7 @@ _zstd_ZstdDecompressor_new_impl(PyTypeObject *type, PyObject *zstd_dict,
         self->dict = zstd_dict;
     }
 
-    /* Set option to decompression context */
+    /* Set options dictionary */
     if (options != Py_None) {
         if (_zstd_set_d_parameters(self, options) < 0) {
             goto error;