search

JohnCremona / eclib

The eclib package includes mwrank (for 2-descent on elliptic curves over Q) and modular symbol code used to create the elliptic curve database.
GNU General Public License v2.0
21 stars 16 forks source link

Fix out-of-bounds vector accesses. #40

Closed jamesjer closed 6 years ago

jamesjer commented 6 years ago

The Fedora project recently added -D_GLIBCXX_ASSERTIONS to the build flags. With recent versions of glibc, this activates runtime assertions in the C++ library. Building the eclib package with the assertions activated revealed two places in the code where an out-of-bounds vector index is accessed, both in libsrc/symb.cc. You may well want to address the issue in some other way than the approach I took with this pull request, but it at least shows you where in the code these accesses are happening.

In the first case, on line 157, a vector element is assigned to, but may not exist already. The assign method both does the assignment and increases the size of the vector so that the vector is valid.

In the second case, on line 187, the value of noninvdlist[-kc] can be larger than the size of the dstarts array; I chose to set start to zero.

The testsuite runs successfully with these two changes.

JohnCremona commented 6 years ago

Thanks for this. I'll look at it carefully.

JohnCremona commented 6 years ago

I have looked at both these carefully: in both cases if the code is doing what it is supposed to then there would not be any out-of-bounds accesses, so I put in some print statements and found what was going on: in the line in moddata.cc I changed dstarts.reserve(ndivs) to use resize() instead. This should mean that the changes in this patch are not needed. The new code is in the includes branch which I will merge when Travis tests pass.

Here is my analysis, for the record. Line 157 of symb.cc assigns to dstarts[ic] for 0 <= ic <= ndivs-1. The array dstarts is declared (as a member of class moddata) in moddata.h and then on line 134 of moddata.cc it has its size "reserved" at ndivs. I thought that was OK, but I have changed the dstarts.reserve(ndivs) to dstarts.resize(ndivs) and that seems to fix this one. ndivs is computed on line 32 of moddata.cc and never changes.

Line 187 accesses dstarts at index noninvdlist[-kc], so this relies on that being between 0 and ndivs-1 inclusive. The values of noninvdlist are set on line 98 of moddata.cc.

JohnCremona commented 6 years ago

Fixed elsewhere