Fix int32 overflows in sparsetools #3224

Merged
merged 5 commits into from Feb 1, 2014

Conversation

Projects
None yet
4 participants
Owner

pv commented Jan 19, 2014

The sparsetools code has several int32 overflows on 64-bit systems:

  • Some of the sparsetools routines index into 2D dense arrays. The indices into these are not bounded by nnz, and can overflow int32 limits.
  • The nnz of BSR is not bounded by the index data type, which only bounds the number of blocks. The block size may easily overflow int32.

This PR contains some tests that require a lot of memory, so we check the available amount first (doing this on Linux should be enough). Some of the tests are also very slow (~ minutes), so they are enabled only if an environment variable is set.

Fixes gh-3212, gh-2179

@pv pv BUG: sparse: fix some integer wraparounds in sparsetools
Some of the sparsetools routines index into 2D dense arrays.  The
indices into these are not bounded by nnz, and can overflow int32
limits. Take care of this.
3b36a57

Coverage Status

Coverage remained the same when pulling 7fa9591 on pv:sparsetools-overflow into dc7555b on scipy:master.

@rgommers rgommers commented on an outdated diff Jan 22, 2014

scipy/sparse/sparsetools/bsr.h
@@ -213,7 +214,7 @@ void bsr_sort_indices(const I n_brow,
*/
template <class I, class T>
void bsr_transpose(const I n_brow,
- const I n_bcol,
+ const I n_bcol,
@rgommers

rgommers Jan 22, 2014

Owner

Indentation issue here

Owner

rgommers commented Jan 22, 2014

Tested also on 32-bit, works fine.

@rgommers rgommers commented on the diff Jan 22, 2014

scipy/sparse/tests/test_sparsetools.py
+from numpy.testing import assert_raises, assert_equal, dec, run_module_suite
+from scipy.sparse import (sparsetools, coo_matrix, csr_matrix, csc_matrix,
+ bsr_matrix, dia_matrix)
+from scipy.lib.decorator import decorator
+
+
+@decorator
+def xslow(func, *a, **kw):
+ try:
+ v = int(os.environ.get('SCIPY_XSLOW', '0'))
+ if not v:
+ raise ValueError()
+ except ValueError:
+ raise SkipTest("very slow test; set environment variable "
+ "SCIPY_XSLOW=1 to run it")
+ return func(*a, **kw)
@rgommers

rgommers Jan 22, 2014

Owner

You put in the try-except in order to not get an error when SCIPY_XSLOW='unconvertible_to_int', right? Not sure if that helps, may at least print a warning in that case.

@pv

pv Jan 25, 2014

Owner

Yes. I think it's clear enough if the test just prints the above message, as it tells explicitly what to do to enable the test.

pv added some commits Jan 18, 2014

@pv pv BUG: sparse: fix int32 wraparounds in BSR matrices
The issue here is that the nnz of BSR is not bounded by the index data
type, which only bounds the number of blocks.  The block size may easily
overflow int32.
9dfd453
@pv pv BUG: sparse: fix integer overflow in CSR matmat 8003427
@pv pv TST: sparse: add tests probing for int32 overflows 357375c
Owner

pv commented Jan 25, 2014

Fixed the spurious whitespace changes.

Coverage Status

Coverage remained the same when pulling 357375c on pv:sparsetools-overflow into d93194e on scipy:master.

Owner

pv commented Jan 25, 2014

I'd like to merge (this causes conflicts in gh-442). Further comments?

Member

WarrenWeckesser commented Jan 25, 2014

I'm trying it now...

Member

WarrenWeckesser commented Jan 25, 2014

The use of the environment variable SCIPY_XSLOW should be documented... somewhere. As it is, I don't see how anyone would know about it unless they read this PR or stumbled across in the test file.

Owner

pv commented Jan 25, 2014

It's printed in the test output, too.

Member

WarrenWeckesser commented Jan 25, 2014

I don't see any mention of it in the output of

python -c "from scipy import sparse; sparse.test('full')"
Owner

pv commented Jan 25, 2014

Needs verbose=1 --- skipped tests usually print an explanation why they are skipped.

Owner

pv commented Jan 25, 2014

Added description to HACKING.txt

Coverage Status

Coverage remained the same when pulling 8778612 on pv:sparsetools-overflow into 8ad48cd on scipy:master.

Coverage Status

Coverage remained the same when pulling 8778612 on pv:sparsetools-overflow into 8ad48cd on scipy:master.

Owner

pv commented Feb 1, 2014

Comments addressed, so merging.

@pv pv added a commit that referenced this pull request Feb 1, 2014

@pv pv Merge pull request #3224 from pv/sparsetools-overflow
BUG: sparse: fix int32 overflows in sparsetools
c35429e

@pv pv merged commit c35429e into scipy:master Feb 1, 2014

1 check passed

default The Travis CI build passed
Details

rgommers added this to the 0.14.0 milestone Feb 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment