fix overflow in Matrix_modn_dense

[videlec]

Replace use of unsigned int in matrix indices in favor of Py_ssize_t to avoid overflow.

Fixes #35885

[github-actions]

Documentation preview for this PR (built with commit 36b43a4; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[dimpase]

please rebase

[schmittj]

I tried cloning the repo and running the test, but it did not fix the issues:

[schmittj@ada-21:sage] $ nice -n 7 ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.5.beta4, Release Date: 2024-09-15              │
│ Using Python 3.12.9. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: sage: F = GF(8388593)
sage: sage: M = matrix(F, 65537, 65537)
sage: sage: M[1,0]
0
sage: sage: M[65536,1]=1
sage: sage: M[1,0]
1
sage: sage: M[65536,1]
1

I pulled the code as follows:

 git clone https://github.com/videlec/sage.git
 git checkout matrix-indices-overflow

What is strange to me is that the Files changed tab above just lists some changes to LattE executables. Did the rebase do what it was supposed to do?

[schmittj]

My impression is that the changes from commit d16f956 were lost in the rebase.

See the current file
https://github.com/videlec/sage/blob/matrix-indices-overflow/src/sage/matrix/matrix_modn_dense_template.pxi
which still has the

        cdef unsigned int k
        cdef Py_ssize_t i
        k = 0
        for i in range(self._nrows):
            self._matrix[i] = self._entries + k
            k = k + self._ncols

code

[videlec]

My bad. I did a hopefully correct rebase now.

[schmittj]

Thanks a lot! The automated builds still seem to have some issues, but I did manage to build the code locally and it produced the desired result:

sage: def matrixtest(B):
....:     M = matrix(B, 65537, 65537)
....:     a = M[1,0]
....:     M[65536,1]=1
....:     print(a, M[1,0])
....:
sage: for B in [GF(8388593), QQ, ZZ, RR, GF(4), GF(2)]:
....:     matrixtest(B)
....:
0 0
0 0
0 0
0.000000000000000 0.000000000000000
0 0
0 0

I'll also run the code from the original vandermonde-matrix rank problem, but I expect that it will work out likewise.

[videlec]

I am glad that this simple change did it!

[videlec]

@schmittj The CI results look fishy. Did you try to run sage -t on some files locally? I will try to understand what happened later this week.

[schmittj]

First: The Vandermonde check indeed works now giving the correct result.

On the negative side: I agree the CI looks a bit worrying. Running tests locally I do get a lot of segfaults, like the CI build and test:

[schmittj@ada-21:sage] 130 $ ./sage -tp 10 src/sage/matrix/
...
----------------------------------------------------------------------
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_modn_dense_template.pxi  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_integer_dense_hnf.py  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_integer_dense_saturation.py  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_cyclo_dense.pyx  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_integer_dense.pyx  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/tests.py  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_integer_sparse.pyx  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix2.pyx  # Killed due to segmentation fault
sage -t --random-seed=55474474925827698782892552111973673311 src/sage/matrix/matrix_space.py  # Killed due to segmentation fault
----------------------------------------------------------------------

Probably those tests you can also reproduce with your local machine - they don't rely on big RAM.

Thanks a lot for your work and for having a look!

[videlec]

One thing that for sure segfaults is if you ask for a matrix with zero rows. Though I would not expect such a thing to create that many segfaults in doctests.

[dimpase]

Why should asking for a 0-row matrix segfault?

Checking that nrows>0 is very cheap, and throwing a meaningful Python exception should happen instead.

[schmittj]

Probably this entire block should just be inside an

if self._nrows:

environment, because the original code just does not do anything if self._nrows == 0.

I do think that the number of rows should be allowed to be zero, there should be no exception thrown for that. The 0xn matrix is a member in good standing in the big family of matrices!

[videlec]

Note that the SageMath modn matrices carry two pointers: an array _entries of size nrows x ncols and an array _matrix of size nrows. These are redundant as _matrix[i][j] and _entries[i * ncols + j] point to the same thing. But convenient when you manipulate rows. As you suggested, there should be something for the _nrows=0 case which makes the line self._matrix[0] = self._entries invalid.

The more interesting zerology question is whether when ncols=0 we could avoid the allocation of _matrix.

[videlec]

Looks better now. Is it fine for inclusion?

[dimpase]

CI looks good (1 test-long failure is irrelevant here).
However, I wonder whether the whole src/sage/matrix/matrix_modn_dense_template.pxi got to be screened for missing nrows==0 checks. Some functions do this check, some don't...

[videlec]

Not by me. And I would also suggest not to do it in this PR.

[dimpase]

OK. Please flip it to Positive review

[schmittj]

I also compiled the new version again on Fedora and all goes well. I can confirm that it fixes #35885 . Tests went through as well, except for random failures related to some gfan problems that seem unrelated to the changes here (probably my bad when compiling).

[schmittj]

Just to be sure: yes, I approve these changes, from my side the PR is ready to merge!