sagemath/sage

vector matrix multiply causes segfault

roed314 opened this issue · 17 comments

I ran across this while working on overconvergent modular symbols:

sage: vector(1/1) * matrix(Zmod(2),[[1]])
Unhandled SIGSEGV
sage: vector(1/1) * matrix(Zmod(11),[[1]])
Unhandled SIGSEGV

Upstream: Fixed upstream, in a later stable release.

CC: @malb @burcin

Component: linear algebra

Keywords: linbox, segfault

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/12865

Description changed:

--- 
+++ 
@@ -3,6 +3,8 @@
 ```
 sage: vector(1/1) * matrix(Zmod(2),[[1]])
 Unhandled SIGSEGV
+sage: vector(1/1) * matrix(Zmod(11),[[1]])
+Unhandled SIGSEGV
 ```
 
 
comment:2

Backtrace looks interesting:

#0  __strncpy_ssse3 () at ../sysdeps/x86_64/multiarch/strcpy.S:94
#1  0x00007fffd98d3179 in LinboxError (this=0x4aa8100, function=0x7fffd994efe0 "Modular", line=102, 
    check=0x7fffd994ce93 "modulus must be > 1")
    at /data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/include/linbox/util/error.h:41
#2  LinBox::PreconditionFailed::PreconditionFailed (this=0x4aa8100, function=0x7fffd994efe0 "Modular", line=102, 
    check=0x7fffd994ce93 "modulus must be > 1")
    at /data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/include/linbox/util/debug.h:52
...

(This is on Linux x86_64.)

comment:3

(Identical for both examples.)

comment:4

Obviously, this can never have worked, as the LinboxError constructor was (implicitly) called with no arguments (hence msg defaulting to 0 rather than ""):

// util/error.h

...
class LinboxError {
	const static size_t max_error_string = 256;
public:
	LinboxError (const char* msg = 0) {
		std::strncpy(strg, msg, max_error_string);
		strg[max_error_string-1] = 0;
	};
...
// util/debug.h

...
	class PreconditionFailed : public LinboxError
	{
		static std::ostream *_errorStream;

	    public:
		PreconditionFailed (const char *function, int line, const char *check) {
			if (_errorStream == (std::ostream *) 0)
				_errorStream = &std::cerr;

			(*_errorStream) << std::endl << std::endl;
			(*_errorStream) << "ERROR (" << function << ":" << line << "): ";
			(*_errorStream) << "Precondition not met:" << check << std::endl;
		}
...

From LinBox 1.1.7:

...
class LinboxError {
        static const size_t max_error_string = 256;
public:
        LinboxError (const char* msg = '\0') {
                std::strncpy(strg, msg, max_error_string);
                strg[max_error_string-1] = 0;
        };
...

Upstream: Fixed upstream, in a later stable release.

comment:5

... although the "fix" in 1.1.7 is also plain wrong, as it should be const char *msg = "", not = '\0', since the latter again yields a NULL pointer, which then causes strncpy() to segfault.

comment:6

Replying to @nexttime:

... although the "fix" in 1.1.7 is also plain wrong, as it should be const char *msg = "", not = '\0', since the latter again yields a NULL pointer, which then causes strncpy() to segfault.

But they've "fixed" it in another way... XD

...
        class PreconditionFailed //: public LinboxError BB: otherwise,  error.h:39 segfaults
        {
...

OMG.

comment:7

FWIW, the latest development release, LinBox 1.2.2, still has this !! in it.

I'll probably add a proper fix to my LinBox 1.1.6 spkg at #12762.

comment:8

With my fix, I now get a "proper" error:

sage: vector(1/1) * matrix(Zmod(2),[[1]])


ERROR (Modular:102): Precondition not met:modulus must be > 1
terminate called after throwing an instance of 'LinBox::PreconditionFailed'
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/libcsage.so(print_backtrace+0x31)[0x7fb1d13113a3]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/libcsage.so(sigdie+0x14)[0x7fb1d13113d5]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/libcsage.so(sage_signal_handler+0x1d5)[0x7fb1d1310fc7]
/lib/libpthread.so.0(+0xf8f0)[0x7fb1d62e68f0]
/lib/libc.so.6(gsignal+0x35)[0x7fb1d58fda75]
/lib/libc.so.6(abort+0x180)[0x7fb1d59015c0]
/usr/lib/libstdc++.so.6(_ZN9__gnu_cxx27__verbose_terminate_handlerEv+0x125)[0x7fb1d0dc0285]
/usr/lib/libstdc++.so.6(+0x5d1a6)[0x7fb1d0dbe1a6]
/usr/lib/libstdc++.so.6(+0x5d1d3)[0x7fb1d0dbe1d3]
/usr/lib/libstdc++.so.6(+0x5d47e)[0x7fb1d0dbe47e]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/matrix/matrix_modn_dense_float.so(_ZN6LinBox7ModularIfEC1El+0xc8)[0x7fb1b85194a8]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/matrix/matrix_modn_dense_float.so(+0x2cf4a)[0x7fb1b8507f4a]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/matrix/matrix_modn_dense_float.so(+0x2fb3b)[0x7fb1b850ab3b]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/matrix/action.so(+0x382b)[0x7fb1a510982b]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/structure/coerce.so(+0xf632)[0x7fb1cd5f3632]
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/local/lib/python2.7/site-packages/sage/structure/element.so(+0x1e66c)[0x7fb1cd83766c]
...
/lib/libc.so.6(__libc_start_main+0xfd)[0x7fb1d58e8c4d]
python[0x4006d1]

------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off(). You might
want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate.
------------------------------------------------------------------------
/data/leif/Sage/sage-5.0.beta10-gcc-4.7.0/spkg/bin/sage: line 308: 13087 Aborted                 sage-ipython "$@" -i

Certainly the Sage library has to get fixed as well.

comment:9

Since you both worked on #4260 (merged into Sage 4.8.alpha3), you probably know how to fix this...

----------------------------------------------------------------------
| Sage Version 4.7.2, Release Date: 2011-10-29                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: vector(1/1) * matrix(Zmod(2),[[1]])
(0)
sage: vector(1/1) * matrix(Zmod(11),[[1]])
(0)

With my limited knowledge, I'd rather expect (1) in both cases.

comment:10

I think it should raise a TypeError, since there's no pushout between the rational field and Zmod(11):

sage: from sage.categories.pushout import pushout
sage: pushout(QQ,Zmod(11))
Ring of integers modulo 1
sage: QQ(1) * Zmod(11)(1)
...
TypeError: unsupported operand parent(s) for '*': 'Rational Field' and 'Ring of integers modulo 11'
comment:11

So there are actually three things to fix:

  • The LinboxError constructor (and/or the Linbox::PreconditionFailed constructor).

  • The modulus has to be checked before calling LinBox, and modulus 1 has to be trivially(?) treated differently, as all of

sage: v=vector([Zmod(1)(1)]); m=matrix(Zmod(1),[[1]])
sage: v*m
sage: m*v
sage: m*m

currently fail (while they worked in Sage 4.7.2).

  • According to you, the coercion is wrong and should not happen at all.

    (I never really understood the rationale of Sage's coercion model, as from the perspective of programming languages or type theory, any coercion -- as an implicit, automatic type conversion or cast -- should be a type promotion. And I have no idea how it comes to the ring of integers modulo 1.)

I can fix the first issue (e.g. at #12762); the others, especially the last one, have to get fixed by someone else.

comment:12

This is apparently fixed by #12883:

sage: vector(1/1) * matrix(Zmod(2),[[1]])
(0)
sage: vector(1/1) * matrix(Zmod(11),[[1]])
(0)
sage: sage: v=vector([Zmod(1)(1)]); m=matrix(Zmod(1),[[1]])
sage: sage: v*m
(0)
sage: sage: m*v
(0)
sage: sage: m*m
[0]
comment:13

Replying to @vbraun:

This is apparently fixed by #12883

Should we then add according doctests [here]?

Don't know whether what David mentioned is also properly addressed.

comment:19
sage: vector(1/1) * matrix(Zmod(2),[[1]])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-802e5729e726> in <module>()
----> 1 vector(Integer(1)/Integer(1)) * matrix(Zmod(Integer(2)),[[Integer(1)]])

/home/darij/sage-6.3.beta6/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Vector.__mul__ (build/cythonized/sage/structure/element.c:20577)()

/home/darij/sage-6.3.beta6/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:8764)()

TypeError: unsupported operand parent(s) for '*': 'Vector space of dimension 1 over Rational Field' and 'Full MatrixSpace of 1 by 1 dense matrices over Ring of integers modulo 2'
sage: vector(1/1) * matrix(Zmod(11),[[1]])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-9ab2f0f0d5e1> in <module>()
----> 1 vector(Integer(1)/Integer(1)) * matrix(Zmod(Integer(11)),[[Integer(1)]])

/home/darij/sage-6.3.beta6/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Vector.__mul__ (build/cythonized/sage/structure/element.c:20577)()

/home/darij/sage-6.3.beta6/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:8764)()

TypeError: unsupported operand parent(s) for '*': 'Vector space of dimension 1 over Rational Field' and 'Full MatrixSpace of 1 by 1 dense matrices over Ring of integers modulo 11'

So, obsolete?

comment:20

Not obsolete... On sage-6.6.beta1 I got it possible again

sage: vector(1/1) * matrix(Zmod(2),[[1]])
(1)

Something has changed in the coercion model and I opened #17859 for that (see also comment:11 by David which already pointed out the issue).

I propose to move this ticket to fixed since it is ok with Linbox.

Reviewer: Jeroen Demeyer