Macaulay2 / M2

The primary source code repository for Macaulay2, a system for computing in commutative algebra, algebraic geometry and related fields.
https://macaulay2.com
330 stars 226 forks source link

Vector overrun in monoid.cpp #3316

Closed jamesjer closed 6 days ago

jamesjer commented 1 week ago

The Fedora project builds packages with -D_GLIBCXX_ASSERTIONS, which enables various assertions in gcc's C++ headers. I just did a test build of version 1.24.05 and one such assertion failed:

/usr/include/c++/14/bits/stl_vector.h:1225: std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::front() const [with _Tp = int; _Alloc = std::allocator<int>; const_reference = const int&]: Assertion '!this->empty()' failed.

The assertion is triggered on line 191 of monoid.cpp, on this line in Monoid::set_degrees():

  auto *iter = &mDegrees.front();

In this case, mDegrees is empty, so the call to the front method is illegal. This can be worked around as follows:

  auto *iter = mDegrees.data();

However, that just silences the assertion. It doesn't solve the underlying issue, which is that in this case, iter is a pointer that cannot be dereferenced safely. That is the answer to the question on line 212:

      // TODO: what is iter in this case?!

Perhaps iter should be set to nullptr in this case, so that attempts to dereference it will trigger a segfault. What will happen on line 214 when mDegreeMonoid->from_expvector(iter, m) is called, and iter is a pointer that cannot be dereferenced?

mahrud commented 1 week ago

That's a good flag to add to the debug build!

That is the answer to the question on line 212:

Thanks for answering my question 😆 I opened a PR that should fix this issue when defining the trivial monoid.

jamesjer commented 1 week ago

FYI, that flag has turned up a completely different vector overrun. This is M2/Macaulay2/e/BasicPolyListParser.cpp line 85:

      while ((isalpha(line[loc]) or isdigit(line[loc]) or line[loc] == '_') and (loc < line.size()))

That is, first the loc element of a vector is accessed (up to 3 times), and only then do we check that loc is a valid index. This triggered an assertion failure in a test, while processing the string "x, y, z". The two conjuncts should be reversed.

mahrud commented 1 week ago

Which test triggered this?

jamesjer commented 1 week ago
[ RUN      ] MatrixIO.readMsolve
/usr/include/c++/14/string_view:256: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.
Command terminated by signal 6