Skip to content

Check that imported values have an element for every entry - #643

Open
eriknw wants to merge 2 commits into
47b-udt-values-lengthfrom
47c-import-values-length
Open

eriknw wants to merge 2 commits into
47b-udt-values-lengthfrom
47c-import-values-length

Conversation

@eriknw

@eriknw eriknw commented Oct 19, 2026

Copy link
Copy Markdown
Member

SuiteSparse checks the size of an imported values buffer in the wrong units. GB_import
sets Ax_memsize_for_non_iso to nvals (sparse, hypersparse) or vlen * vdim (bitmap,
full), a count of entries. It then compares that count with Ax_memsize, which is in bytes,
as documented and as python-graphblas passes it (values.nbytes). So any buffer of at least
nvals bytes is accepted, and later reads run past its end:

Vector.ss.import_full(values=np.ones(3), size=5)

returned a Vector whose last two values were heap garbage. The same happened with values too
short for a sub-array dtype:

Vector.ss.import_sparse(size=3, indices=[0, 1], values=np.ones(2), dtype="FP64[2]")

The previous PR in the stack, which checks for whole elements in build, left these
imports for a follow-up.

Every ss import and pack now checks the values before handing them to GraphBLAS: sparse,
bitmap and full Vectors, and csr, csc, hypercsr, hypercsc, bitmapr, bitmapc, fullr and
fullc Matrices, which also covers Matrix.from_dcsr and from_dcsc on SuiteSparse. They
raise

ValueError: values must have 5 or more elements of dtype FP64, not 3

The check is one helper, check_values_size, next to 47b's check_element_shape. It counts
what GraphBLAS will read: nvals, indptr[n], the size, or nrows x ncols. An iso import needs
one element, so iso imports with a single value still work. A larger buffer is still
accepted.

test_ss_import_export imported a 14 x 7 bitmap with 7 x 7 values, so GraphBLAS read 49
values past their end. It now passes values for the whole bitmap and checks the second
half. The new tests (test_ss_import_values_size in test_vector.py and test_matrix.py,
SuiteSparse only) cover FP64 and FP64[2], sparse, bitmap and full formats, an import and a
pack, and iso imports given one element. Both fail on the parent commit.

The SuiteSparse side is written up as a C MRE with a small fix that multiplies by
type->size (not filed yet). It reproduces on 10.5.0, 10.5.1 and the current stable; the
faulty lines date back to at least 7.4.0.

@eriknw
eriknw added this pull request to stack #650 October 19, 2026 07:50
@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.65%. Comparing base (8a00ef4) to head (cdfaec6).

Additional details and impacted files
@@                    Coverage Diff                    @@
##           47b-udt-values-length     #643      +/-   ##
=========================================================
+ Coverage                  98.64%   98.65%   +0.01%     
=========================================================
  Files                        105      105              
  Lines                      29443    29510      +67     
  Branches                    3449     3452       +3     
=========================================================
+ Hits                       29043    29113      +70     
  Misses                       261      261              
+ Partials                     139      136       -3     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eriknw
eriknw force-pushed the 47c-import-values-length branch from d4703df to f11c703 Compare October 19, 2026 07:55
@eriknw
eriknw force-pushed the 47c-import-values-length branch from f11c703 to 2e69171 Compare October 20, 2026 22:28
@eriknw
eriknw force-pushed the 47c-import-values-length branch from 2e69171 to 1e9240e Compare October 21, 2026 04:14
@eriknw
eriknw force-pushed the 47c-import-values-length branch 2 times, most recently from 0e0e155 to f5d2961 Compare October 28, 2026 04:29
@eriknw
eriknw removed this pull request from stack #650 October 28, 2026 04:29
@eriknw
eriknw added this pull request to stack #653 October 28, 2026 04:30
@eriknw
eriknw removed this pull request from stack #653 October 28, 2026 04:31
@eriknw
eriknw added this pull request to stack #655 October 28, 2026 04:32
@eriknw
eriknw removed this pull request from stack #655 October 28, 2026 04:33
@eriknw
eriknw added this pull request to stack #657 October 28, 2026 04:33
@eriknw
eriknw force-pushed the 47c-import-values-length branch from f5d2961 to 842d327 Compare October 28, 2026 13:21
SuiteSparse compares the byte size of an imported values buffer with the
number of entries, not with the bytes they need (GB_import.c sets
Ax_memsize_for_non_iso to nvals or vlen * vdim), so any buffer of at least
nvals bytes got through and was read past its end:

    Vector.ss.import_full(values=np.ones(3), size=5)

built a Vector whose last two values were heap garbage, and so did values
too short for a sub-array dtype, as in

    Vector.ss.import_sparse(size=3, indices=[0, 1], values=np.ones(2), dtype="FP64[2]")

Every ss import and pack (sparse, bitmap and full Vectors; csr, csc,
hypercsr, hypercsc, bitmapr, bitmapc, fullr and fullc Matrices, and so
from_dcsr and from_dcsc) now raises

    ValueError: values must have 5 or more elements of dtype FP64, not 3

before handing GraphBLAS the buffer. An iso import needs one element.
…lues

SuiteSparse:GraphBLAS 10.5.2 fixed the import size check that
check_values_size works around: GB_import.c compared the byte size of
the values with the number of entries. With check_values_size bypassed,
the five short imports tried (import_full, import_sparse of FP64[2],
import_bitmapr, and two import_csr) now raise InvalidValue with an empty
message on 10.5.2, where 10.5.1 built them from bytes past the end.
check_values_size still runs on every version: older ones need it, and
its ValueError says what is wrong. Its docstring and the comments of
the two test_ss_import_values_size tests now give the versions.
Comments only.
@eriknw
eriknw force-pushed the 47c-import-values-length branch from 842d327 to cdfaec6 Compare October 29, 2026 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant