chunkers: review fixes (chunking_time precision, param checks, cleanups)

src/borg/chunkers/base.pxd

@@ -18,7 +18,7 @@ cdef class ChunkerBase:
     cdef int done, eof
     cdef size_t min_size, buf_size, remaining, position, last
     cdef long long bytes_read, bytes_yielded
-    cdef readonly float chunking_time
+    cdef readonly double chunking_time  # a sum of many small increments, a C float would stop growing
     cdef object file_reader
     cdef size_t reader_block_size
     cdef bint sparse

src/borg/chunkers/base.pyx

@@ -50,8 +50,12 @@ cdef class ChunkerBase:
         """
         min_size = 1 << chunk_min_exp
         max_size = 1 << chunk_max_exp
-        assert max_size <= len(zeros)
-        assert min_size + 1 <= max_size, "too small max_size"
+        # The parameter checks here and in the subclasses raise ValueError rather than
+        # assert: the scan code relies on them for memory safety and python -O drops asserts.
+        if max_size > len(zeros):
+            raise ValueError("too big max_size")
+        if min_size + 1 > max_size:
+            raise ValueError("too small max_size")
 
         self.algo = algo
         self.chunk_mask = _mask_bits(hash_mask_bits, high_masks)
@@ -61,9 +65,10 @@ cdef class ChunkerBase:
         # probability). This concentrates chunk sizes around the target and reduces
         # chunk-size variance. nc_level == 0 disables it, keeping behavior byte-identical
         # to the single-mask chunker.
-        assert nc_level >= 0
-        assert hash_mask_bits - nc_level >= 1, "nc_level too large for hash_mask_bits"
-        assert hash_mask_bits + nc_level <= 48, "nc_level too large for hash_mask_bits"
+        if nc_level < 0:
+            raise ValueError("nc_level must not be negative")
+        if hash_mask_bits - nc_level < 1 or hash_mask_bits + nc_level > 48:
+            raise ValueError("nc_level too large for hash_mask_bits")
         self.nc_level = nc_level
         if nc_level:
             self.mask_s = _mask_bits(hash_mask_bits + nc_level, high_masks)
@@ -152,10 +157,9 @@ cdef class ChunkerBase:
         cdef uint64_t digest = 0, mask, mask_s = self.mask_s, mask_l = self.mask_l
         cdef int nc_level = self.nc_level
         cdef size_t n, old_last, min_size = self.min_size
-        cdef size_t normal_size = self.normal_size, normal_pos, chunk_len, span, did
+        cdef size_t normal_size = self.normal_size, chunk_len, span, did
         cdef int64_t r
         cdef uint8_t* p
-        cdef uint8_t* stop
 
         if self.done:
             if self.bytes_read == self.bytes_yielded:
@@ -202,14 +206,13 @@ cdef class ChunkerBase:
                 continue
 
             p = self.data + self.position
-            stop = p + self.remaining
-            if nc_level and chunk_len < normal_size:
-                # do not scan past the strict->loose transition; re-evaluate the mask there
-                normal_pos = self.last + normal_size
-                if (self.data + normal_pos) < stop:
-                    stop = self.data + normal_pos
-
-            span = stop - p
+            span = self.remaining
+            if nc_level and chunk_len < normal_size and normal_size - chunk_len < span:
+                # do not scan past the strict->loose transition; re-evaluate the mask there.
+                # This compares lengths, not positions: normal_size is a caller-given size_t
+                # and can be anything, self.last + normal_size might not even be representable.
+                span = normal_size - chunk_len
+
             r = self._scan(p, span, &digest, mask)
             if r == -2:
                 raise Exception(f"{self.algo}: OpenSSL EVP encryption failed")

src/borg/chunkers/buzhash.pyx

@@ -138,7 +138,10 @@ cdef class Chunker(ChunkerBase):
     def __cinit__(self, int seed, int chunk_min_exp, int chunk_max_exp, int hash_mask_bits, int hash_window_size, bint sparse=False):
         self.table = NULL
         # see process, first while loop condition, first term must be able to get True:
-        assert hash_window_size + (1 << chunk_min_exp) + 1 <= (1 << chunk_max_exp), "too small max_size"
+        if hash_window_size < 0:
+            raise ValueError("hash_window_size must not be negative")
+        if hash_window_size + (1 << chunk_min_exp) + 1 > (1 << chunk_max_exp):
+            raise ValueError("too small max_size")
         self.window_size = hash_window_size
         self.table = buzhash_init_table(seed & 0xffffffff)
         # buzhash is single-mask (no normalized chunking): nc_level 0 makes
@@ -167,7 +170,7 @@ cdef class Chunker(ChunkerBase):
             else:
                 raise Exception("chunkifier byte count mismatch")
 
-        while self.remaining < min_size + window_size + 1 and not self.eof:  # see assert in Chunker init
+        while self.remaining < min_size + window_size + 1 and not self.eof:  # see check in __cinit__
             self.fill()
 
         # Here we either are at eof...

src/borg/chunkers/buzhash64.pyx

@@ -150,7 +150,10 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         self.table = NULL
         self.table_rot = NULL
         # see process, first while loop condition, first term must be able to get True:
-        assert hash_window_size + (1 << chunk_min_exp) + 1 <= (1 << chunk_max_exp), "too small max_size"
+        if hash_window_size < 0:
+            raise ValueError("hash_window_size must not be negative")
+        if hash_window_size + (1 << chunk_min_exp) + 1 > (1 << chunk_max_exp):
+            raise ValueError("too small max_size")
 
         self.window_size = hash_window_size
         self.table = buzhash64_init_table(key)
@@ -191,10 +194,8 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         cdef uint64_t mask_s = self.mask_s, mask_l = self.mask_l
         cdef int nc_level = self.nc_level
         cdef size_t n, old_last, min_size = self.min_size, window_size = self.window_size
-        cdef size_t normal_size = self.normal_size, normal_pos
+        cdef size_t normal_size = self.normal_size, chunk_len
         cdef uint8_t* p
-        cdef uint8_t* stop_at
-        cdef uint8_t* nc_stop
         cdef size_t did_bytes, span
         cdef int kernel_id = self.kernel_id
 
@@ -204,7 +205,7 @@ cdef class ChunkerBuzHash64(ChunkerBase):
             else:
                 raise Exception("chunkifier byte count mismatch")
 
-        while self.remaining < min_size + window_size + 1 and not self.eof:  # see assert in Chunker init
+        while self.remaining < min_size + window_size + 1 and not self.eof:  # see check in __cinit__
             self.fill()
 
         # Here we either are at eof...
@@ -231,29 +232,28 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         # Normalized chunking: pick the mask based on how far we are into the current chunk.
         # While below normal_size use the strict mask (lower cut probability), afterward the
         # loose mask (higher cut probability). The mask is re-evaluated at the top of every
-        # iteration, so the transition is honored exactly at normal_pos. When nc is disabled,
-        # mask_s == mask_l == chunk_mask and the normal_pos cap is not applied, so this reduces
+        # iteration, so the transition is honored exactly at normal_size. When nc is disabled,
+        # mask_s == mask_l == chunk_mask and the normal_size cap is not applied, so this reduces
         # to the original single-mask behavior.
         mask = mask_s
-        normal_pos = 0
+        chunk_len = 0
         while True:
             if nc_level:
-                normal_pos = self.last + normal_size
-                mask = mask_s if self.position < normal_pos else mask_l
+                chunk_len = self.position - self.last
+                mask = mask_s if chunk_len < normal_size else mask_l
 
             if not (self.remaining > window_size and (sum & mask) and not (self.eof and self.remaining <= window_size)):
                 break
 
             p = self.data + self.position
-            stop_at = p + self.remaining - window_size
+            span = self.remaining - window_size
 
-            if nc_level and self.position < normal_pos:
-                # do not scan past the strict->loose transition; re-evaluate the mask there
-                nc_stop = self.data + normal_pos
-                if nc_stop < stop_at:
-                    stop_at = nc_stop
+            if nc_level and chunk_len < normal_size and normal_size - chunk_len < span:
+                # do not scan past the strict->loose transition; re-evaluate the mask there.
+                # This compares lengths, not positions: normal_size is a caller-given size_t
+                # and can be anything, self.last + normal_size might not even be representable.
+                span = normal_size - chunk_len
 
-            span = stop_at - p
             with nogil:
                 did_bytes = bz64_scan(self.table, self.table_rot, p, p + window_size,
                                       span, &sum, mask, kernel_id)

src/borg/chunkers/buzhash64_impl.c

@@ -428,14 +428,10 @@ int bz64_kernel_select(const char *name, int *out_id)
         return BZ_KSEL_OK;
     }
     if (strcmp(name, "avx512") == 0) {
-#ifndef BZ_KIND_512
-        return BZ_KSEL_NOTBUILT; /* compiler too old for the target attribute */
-#else
         if (!__builtin_cpu_supports("avx512f"))
             return BZ_KSEL_NOCPU;
         *out_id = BZ_K_VECTOR512;
         return BZ_KSEL_OK;
-#endif
     }
 #endif
     return BZ_KSEL_UNKNOWN;

src/borg/chunkers/buzhash64_impl.h

@@ -32,7 +32,7 @@
 /* Results of bz64_kernel_select(). */
 #define BZ_KSEL_OK 0
 #define BZ_KSEL_UNKNOWN 1  /* not a kernel name on this platform */
-#define BZ_KSEL_NOTBUILT 2 /* known, but not compiled into this binary */
+#define BZ_KSEL_NOTBUILT 2 /* known, but not compiled into this binary (never returned, see fastcdc_impl.h) */
 #define BZ_KSEL_NOCPU 3    /* known and built, but this CPU cannot run it */
 
 /* Resolve a kernel name for this build; see fc_kernel_select(). */

src/borg/chunkers/fastcdc_impl.c

@@ -409,14 +409,10 @@ int fc_kernel_select(const char *name, int *out_id)
         return FC_KSEL_OK;
     }
     if (strcmp(name, "avx512") == 0) {
-#ifndef FC_KIND_512
-        return FC_KSEL_NOTBUILT; /* compiler too old for the target attribute */
-#else
         if (!__builtin_cpu_supports("avx512f"))
             return FC_KSEL_NOCPU;
         *out_id = FC_K_VECTOR512;
         return FC_KSEL_OK;
-#endif
     }
 #endif
     return FC_KSEL_UNKNOWN;

src/borg/chunkers/fastcdc_impl.h

@@ -28,13 +28,16 @@
 /* Results of fc_kernel_select(). */
 #define FC_KSEL_OK 0
 #define FC_KSEL_UNKNOWN 1  /* not a kernel name on this platform */
-#define FC_KSEL_NOTBUILT 2 /* known, but not compiled into this binary */
+#define FC_KSEL_NOTBUILT 2 /* known, but not compiled into this binary (see below) */
 #define FC_KSEL_NOCPU 3    /* known and built, but this CPU cannot run it */
 
 /* Resolve a kernel name for this build. On FC_KSEL_OK the id is stored in
- * *out_id, otherwise *out_id is left alone. The three failures are kept apart
- * because they need different fixes: a typo, too old a compiler, or the wrong
- * CPU. */
+ * *out_id, otherwise *out_id is left alone. The failures are kept apart
+ * because they need different fixes: a typo or the wrong CPU.
+ * FC_KSEL_NOTBUILT (too old a compiler) is never returned: a compiler that can
+ * build this file at all builds all of a platform's kernels. The code exists
+ * because the kernel selectors share their result codes (see kernel_env.py)
+ * and the AES chunkers' selector does return it. */
 int fc_kernel_select(const char *name, int *out_id);
 
 /* Comma-separated list of the kernel names this build accepts, for error

src/borg/chunkers/goldilocks_aes_impl.c

@@ -171,7 +171,7 @@ uint64_t gl_digest64(const GL_CTX *c, const uint8_t *q)
 #define PH_DIGEST64 gl_digest64
 #include "phte_scan.h"
 
-GL_CTX *gl_new(const uint64_t tables[GL_TABLES * 256], uint64_t k1, uint64_t k2,
+GL_CTX *gl_new(const uint64_t *tables, uint64_t k1, uint64_t k2,
                const uint8_t aes_key[16], int kernel)
 {
     GL_CTX *c = calloc(1, sizeof(GL_CTX));

src/borg/chunkers/phte_chunker.pyx

@@ -33,7 +33,8 @@ cdef class ChunkerPHTE(ChunkerBase):
         # the rolling window needs 64 bytes of in-chunk history at the first
         # possible cut position (min_size), so the minimum chunk size must be
         # at least the window size:
-        assert chunk_min_exp >= 6, "chunk_min_exp must be >= 6 (2^6 = 64 = window size)"
+        if chunk_min_exp < 6:
+            raise ValueError("chunk_min_exp must be >= 6 (2^6 = 64 = window size)")
         self.kernel_str = "unknown"
         # the AES output is uniform, so contiguous low-bit masks are fine
         return self._setup_common(algo, chunk_min_exp, chunk_max_exp, hash_mask_bits,

src/borg/chunkers/phte_core.h

@@ -5,9 +5,12 @@
  * context plumbing are identical. This header holds everything that does not
  * depend on the rolling hash; phte_scan.h holds the scan template that does.
  *
- * Everything here is static (or static inline): each kernel is a separate
- * extension module and gets its own copy, so there is no cross-kernel call,
- * no indirection and no code-generation difference versus hand-copied code.
+ * Everything here is static (or static inline), except for the three
+ * phte_kernel_*() selection functions declared in phte_kernel.h, which the
+ * Cython modules call: each kernel is a separate extension module and gets its
+ * own copy, so there is no cross-kernel call, no indirection and no
+ * code-generation difference versus hand-copied code. For the same reason,
+ * two of the kernels can not be linked into one binary (duplicate symbols).
  */
 
 #ifndef BORG_PHTE_CORE_H

src/borg/chunkers/rabin_aes_impl.c

@@ -66,7 +66,7 @@ uint64_t ra_digest64(const RA_CTX *c, const uint8_t *q)
 #define PH_DIGEST64 ra_digest64
 #include "phte_scan.h"
 
-RA_CTX *ra_new(const uint64_t tables[RA_TABLES * 256], const uint8_t aes_key[16], int kernel)
+RA_CTX *ra_new(const uint64_t *tables, const uint8_t aes_key[16], int kernel)
 {
     RA_CTX *c = calloc(1, sizeof(RA_CTX));
     if (c == NULL)

src/borg/chunkers/toeplitz_aes_impl.c

@@ -84,7 +84,7 @@ uint64_t tp_digest64(const TP_CTX *c, const uint8_t *q)
 #define PH_DIGEST64 tp_digest64
 #include "phte_scan.h"
 
-TP_CTX *tp_new(const uint64_t tables[TP_TABLES * 256], const uint8_t aes_key[16], int kernel)
+TP_CTX *tp_new(const uint64_t *tables, const uint8_t aes_key[16], int kernel)
 {
     TP_CTX *c = calloc(1, sizeof(TP_CTX));
     if (c == NULL)

src/borg/helpers/parseformat.py

@@ -336,8 +336,10 @@ def ChunkerParams(s):
             raise ArgumentTypeError("min. chunk size exponent must not be less than 6 (2^6 = 64B min. chunk size)")
         if chunk_max > 23:
             raise ArgumentTypeError("max. chunk size exponent must not be more than 23 (2^23 = 8MiB max. chunk size)")
+        if window_size < 0:
+            raise ArgumentTypeError("window_size must not be negative")
         # the chunker needs the hash window plus one byte beyond the min. chunk size to fit into
-        # a max. chunk size sized buffer, see the assertion in ChunkerBuzHash64.__cinit__.
+        # a max. chunk size sized buffer, see the check in ChunkerBuzHash64.__cinit__.
         if window_size + 2**chunk_min + 1 > 2**chunk_max:
             raise ArgumentTypeError("required: window_size + 2^chunk_min + 1 <= 2^chunk_max")
         # normalized chunking switches the mask at the target size; it needs room below and above
@@ -364,7 +366,7 @@ def ChunkerParams(s):
         if chunk_max > 23:
             raise ArgumentTypeError("max. chunk size exponent must not be more than 23 (2^23 = 8MiB max. chunk size)")
         # the chunker needs one byte beyond the min. chunk size to fit into a max. chunk size
-        # sized buffer, see the assertion in ChunkerFastCDC.__cinit__.
+        # sized buffer, see the check in ChunkerBase._setup_common.
         if chunk_min >= chunk_max:
             raise ArgumentTypeError("required: chunk_min < chunk_max")
         if not (0 <= nc_level and chunk_mask - nc_level >= 1 and chunk_mask + nc_level <= 48):
@@ -374,7 +376,7 @@ def ChunkerParams(s):
         return CH_FASTCDC, chunk_min, chunk_max, chunk_mask, nc_level
     if algo in (CH_RABIN_AES, CH_GOLDILOCKS_AES, CH_TOEPLITZ_AES):
         # <algo>, chunk_min, chunk_max, chunk_mask, nc_level
-        # both UHF-then-PRF chunkers have a fixed 64-byte window, so there is no window_size field.
+        # the UHF-then-PRF chunkers have a fixed 64-byte window, so there is no window_size field.
         # nc_level is required; use nc_level 0 to disable normalized chunking.
         if count != 5:
             raise ArgumentTypeError(f"{algo} chunker params must be: {algo},chunk_min,chunk_max,chunk_mask,nc_level")
@@ -409,10 +411,12 @@ def ChunkerParams(s):
             raise ArgumentTypeError("min. chunk size exponent must not be less than 6 (2^6 = 64B min. chunk size)")
         if chunk_max > 23:
             raise ArgumentTypeError("max. chunk size exponent must not be more than 23 (2^23 = 8MiB max. chunk size)")
+        if window_size < 0:
+            raise ArgumentTypeError("window_size must not be negative")
         if window_size % 2 == 0:
             raise ArgumentTypeError("window_size must be an uneven (odd) number")
         # the chunker needs the hash window plus one byte beyond the min. chunk size to fit into
-        # a max. chunk size sized buffer, see the assertion in Chunker.__cinit__ (buzhash.pyx).
+        # a max. chunk size sized buffer, see the check in Chunker.__cinit__ (buzhash.pyx).
         if window_size + 2**chunk_min + 1 > 2**chunk_max:
             raise ArgumentTypeError("required: window_size + 2^chunk_min + 1 <= 2^chunk_max")
         return CH_BUZHASH, chunk_min, chunk_max, chunk_mask, window_size

src/borg/testsuite/chunkers/fastcdc_test.py

@@ -270,6 +270,53 @@ def expected_default_kernel(envvar, make, key, monkeypatch):
     raise AssertionError(f"none of {preference} is usable for {envvar} on {machine}")
 
 
+@pytest.mark.parametrize(
+    "algo, params",
+    [
+        (CH_FASTCDC, (10, 10, 10, 0)),  # min. chunk size + 1 does not fit into the max. chunk size
+        (CH_FASTCDC, (10, 25, 14, 0)),  # max. chunk size exceeds MAX_DATA_SIZE
+        (CH_FASTCDC, (10, 16, 14, -1)),  # negative nc_level
+        (CH_FASTCDC, (10, 16, 14, 14)),  # nc_level leaves no bits for the loose mask
+        (CH_BUZHASH, (10, 12, 11, 4095)),  # window + min. chunk size + 1 do not fit into the max. chunk size
+        (CH_BUZHASH, (10, 16, 14, -4095)),  # negative window size
+        (CH_BUZHASH64, (10, 12, 11, 4095, 0)),  # as for buzhash
+        (CH_BUZHASH64, (10, 16, 14, -4095, 0)),  # as for buzhash
+        (CH_TOEPLITZ_AES, (5, 16, 14, 0)),  # min. chunk size smaller than the 64-byte window
+        (CH_RABIN_AES, (5, 16, 14, 0)),
+        (CH_GOLDILOCKS_AES, (5, 16, 14, 0)),
+    ],
+)
+def test_chunkers_reject_invalid_params(algo, params):
+    # The chunkers' own parameter checks protect their scan code from reading outside
+    # the buffer, so they must raise even when asserts are off (python -O) and for
+    # callers that do not go through ChunkerParams. An assert would fail this test with
+    # an AssertionError.
+    with pytest.raises(ValueError):
+        get_chunker(algo, *params, sparse=False)
+
+
+def test_huge_normal_size():
+    # A normal_size that no chunk can reach means that the strict mask is used throughout,
+    # which is the same as a single-mask chunker with nc_level more mask bits.
+    # normal_size is a size_t and may be anything up to SIZE_MAX: the chunkers must not
+    # derive a buffer position from it, that would wrap around and scan outside the buffer.
+    from ...chunkers import ChunkerBuzHash64, ChunkerToeplitzAES
+
+    huge = sys.maxsize * 2 + 1 - 4096  # close to SIZE_MAX
+    data = os.urandom(1024 * 1024)
+
+    def sizes(chunker):
+        return [c.meta["size"] for c in chunker.chunkify(BytesIO(data))]
+
+    assert sizes(ChunkerFastCDC(key0, 10, 16, 12, 2, normal_size=huge)) == sizes(ChunkerFastCDC(key0, 10, 16, 14, 0))
+    assert sizes(ChunkerBuzHash64(key0, 10, 16, 12, 4095, 2, normal_size=huge)) == sizes(
+        ChunkerBuzHash64(key0, 10, 16, 14, 4095, 0)
+    )
+    assert sizes(ChunkerToeplitzAES(key0, 10, 16, 12, 2, normal_size=huge)) == sizes(
+        ChunkerToeplitzAES(key0, 10, 16, 14, 0)
+    )
+
+
 @pytest.mark.parametrize("envvar", ["BORG_FASTCDC_KERNEL", "BORG_BUZHASH64_KERNEL", "BORG_AES_CHUNKER_KERNEL"])
 def test_kernel_env_rejects_unusable(envvar, monkeypatch):
     # A kernel that cannot run here must fail loudly instead of silently