Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

segmentation fault in pexpect for singular #30945

Closed
fchapoton opened this issue Nov 22, 2020 · 30 comments
Closed

segmentation fault in pexpect for singular #30945

fchapoton opened this issue Nov 22, 2020 · 30 comments

Comments

@fchapoton
Copy link
Contributor

At least two patchbots, one Linux and one macOS, meet a failure
related to pexpect interface for Singular, namely:

sage -t --long --random-seed=0 src/sage/interfaces/singular.py  # Killed due to segmentation fault

The tested file also fails when run alone, but the copy-pasted test works fine.

See https://groups.google.com/d/msgid/sage-devel/54c8dd1f-5efa-4a9f-b17f-4225ef9c6c91n%40googlegroups.com.

The following test fails:

sage: try:
    alarm(0.5)
    singular._expect_expr('>')  # interrupt this
except KeyboardInterrupt:
    pass ## line 505 ##
Control-C pressed. Interrupting Singular. Please wait a few seconds...
sage: 2*a ## line 514 ##
------------------------------------------------------------------------
/home/chapoton/sage/local/lib/python3.8/site-packages/cysignals/signals.cpython-38-x86_64-linux-gnu.so(+0x7df4)[0x7f7c33af8df4]
...

For full logs, see:

CC: @dimpase @slel @sagetrac-tmonteil @kiwifb

Component: interfaces

Keywords: singular, segfault

Reviewer: Michael Orlitzky

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

@fchapoton fchapoton added this to the sage-9.3 milestone Nov 22, 2020
@fchapoton
Copy link
Contributor Author

comment:1

changing alarm(0.5) to either alarm(0.5*256) or alarm(0.5/256) does not fix the issue

@slel
Copy link
Member

slel commented Nov 23, 2020

comment:3

I have sometimes seen that failure too.

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Nov 23, 2020

Changed keywords from none to singular, segfault

@fchapoton
Copy link
Contributor Author

comment:4

Je suis preneur de toute suggestion.

@dimpase

This comment has been minimized.

@egourgoulhon
Copy link
Member

comment:6

Some date points, in case it might be useful: on my Ubuntu 20.04 computer, running sage -t --long src/sage/interfaces/singular.py

  • triggers the segfault with Sage 9.3.beta2 and 9.2
  • returns "All tests passed!" with Sage 9.1

@fchapoton
Copy link
Contributor Author

comment:7

pexpect was last updated in #29240, included in Sage 9.2.beta11

@dimpase
Copy link
Member

dimpase commented Nov 26, 2020

comment:8

Perhaps this test failure has nothing to do Singular.

singular's is the only pexpect interface on which this test is carried out. One could also have same for GAP, say:

sage: a = gap(1)
sage: _ = gap._expect.sendline('1+')  # unfinished input
sage: try:
....:     alarm(0.5)
....:     gap._expect_expr('gap>')  # interrupt this
....: except KeyboardInterrupt:
....:     pass                                                                                                                                                                                
Control-C pressed. Interrupting Gap. Please wait a few seconds...
sage: 2*a                                                                                                                                                                      
2

Could someone who has a box where this is reliably reproduced, try adding somewhere the doctest for GAP above, and see if it fails too?

@fchapoton
Copy link
Contributor Author

comment:9

Bug fails with gap too, when running doctests. In the command line, provokes a very bad crash of sage.

running in the doctests:

File "src/sage/interfaces/singular.py", line 511, in sage.interfaces.singular.Singular._send_interrupt
Failed example:
    2*a
Exception raised:
    Traceback (most recent call last):
      File "/home/chapoton/sage/local/lib/python3.8/site-packages/sage/interfaces/gap.py", line 763, in _eval_line
        raise RuntimeError("%s produced error output\n%s\n   executing %s"%(self, error,line))
    RuntimeError: Gap produced error output
    Error, no method found! For debugging hints type ?Recovery from NoMethodFound
    Error, no 1st choice method found for `*' on 2 arguments

       executing \$sage3:=\$sage2 * \$sage1;;

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/chapoton/sage/local/lib/python3.8/site-packages/sage/interfaces/expect.py", line 1469, in __init__
        self._name = parent._create(value, name=name)
      File "/home/chapoton/sage/local/lib/python3.8/site-packages/sage/interfaces/interface.py", line 501, in _create
        self.set(name, value)
      File "/home/chapoton/sage/local/lib/python3.8/site-packages/sage/interfaces/gap.py", line 1422, in set
        self._eval_line(cmd, allow_use_file=True)
      File "/home/chapoton/sage/local/lib/python3.8/site-packages/sage/interfaces/gap.py", line 797, in _eval_line
        raise RuntimeError(exc)
    RuntimeError: Gap produced error output
    Error, no method found! For debugging hints type ?Recovery from NoMethodFound
    Error, no 1st choice method found for `*' on 2 arguments

       executing \$sage3:=\$sage2 * \$sage1;;
etc

in the console, one gets

sage: a                                                                                                                                     
(invalid Gap object -- The gap session in which this object was defined is no longer running.)
sage: 2*a
big mess as above

@dimpase
Copy link
Member

dimpase commented Nov 26, 2020

comment:10

OK, actually I should have checked, for me such GAP pexpect test in Sage console also results in
(invalid Gap object -- The gap session in which this object was defined is no longer running.)

This makes me wonder why it's important to have such a test in Singular. Only Singular has a custom _send_interrupt; GAP
uses the default one, below, and you see it sends _quit_string; it kills the GAP session if it lands in the main loop, rather than in the break loop (doing quit; in GAP's break loop brings you to the main loop, so in this case it's OK).

    def _send_interrupt(self):
        """
        Send an interrupt to the application.  This is used internally
        by :meth:`interrupt`.

        First CTRL-C to stop the current command, then quit.
        """
        self._expect.sendline(chr(3))
        self._expect.sendline(self._quit_string())

Singular custom one was added along with the test in question, and no _quit_string() is sent (which is quit; for GAP, as
you can see by looking at the value of gap._quit_string()).
For Singular one has the following in _send_interrupt():

        sleep(0.1)

        E = self._expect
        E.sendline(chr(3))
        for i in range(5):
            try:
                E.expect_upto(self._prompt, timeout=1.0)
                return
            except Exception:
                pass
            E.sendline(";")

We can try just to erase Singular's _send_interrupt(), set its _quit_string() to proper value (it's quit rather than quit;,
as it should be) and be done with.

@fchapoton
Copy link
Contributor Author

comment:11

here is a tentative, just removing the _send_interrupt method


New commits:

95e1efdremove custom _send_interrupt in singular pexpect interface

@fchapoton
Copy link
Contributor Author

Branch: public/singular_pexpect

@fchapoton
Copy link
Contributor Author

Commit: 95e1efd

@fchapoton
Copy link
Contributor Author

comment:12

Did you mean that exit is the singular quit command ??

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 28, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

85f65bfremove custom _send_interrupt in singular pexpect interface

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 28, 2020

Changed commit from 95e1efd to 85f65bf

@fchapoton
Copy link
Contributor Author

comment:14

hmm, this seems to have a side effect, see the latest patchbot report

@dimpase
Copy link
Member

dimpase commented Nov 29, 2020

comment:15

the side effect could be that hitting ^C during a computation involving a session with pexpect Singular is no longer only (probably) interrupted, but the whole session is killed.

I am not sure it is safe do interrupt Singular - note the warning at the end

^C// ** Interrupt at cmd:`load` in line:';return();'
abort after this command(a), abort immediately(r), print backtrace(b), continue(c) or quit Singular(q) ?// ** Interrupt at cmd:`load` in line:';return();'
abort after this command(a), abort immediately(r), print backtrace(b), continue(c) or quit Singular(q) ?r
** Warning: Singular should be restarted as soon as possible **

in a console Singular session.

@embray
Copy link
Contributor

embray commented Mar 4, 2021

comment:16

I encountered this in #31404 as well. I believe I have isolated it to a possible bug in cysignals: sagemath/cysignals#126

@embray
Copy link
Contributor

embray commented Mar 4, 2021

comment:17

I am able to reproduce this pretty reliably on the current develop branch with

./sage -t --file-iterations=2 -T 0 --verbose src/sage/interfaces/singular.py

@embray
Copy link
Contributor

embray commented Mar 5, 2021

comment:18

This now has a possible fix at sagemath/cysignals#127

The segfault is not caused by Singular (if it were this would crash pexpect, not cause a segfault in Sage). It's caused by cysignals itself.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 16, 2021

Dependencies: #31474

@slel
Copy link
Member

slel commented Apr 1, 2021

Changed commit from 85f65bf to none

@slel
Copy link
Member

slel commented Apr 1, 2021

Changed branch from public/singular_pexpect to none

@slel
Copy link
Member

slel commented Apr 1, 2021

comment:21

Likely solved by cysignals 1.10.3 upgrade at #31474,
merged in Sage 9.3.rc0, released 2021-03-24.

If we get no more reports on sage-release in a while,
this ticket should be closed.

Can the branch public/singular_pexpect improving
interfaces/singular.py go to a new ticket?

@slel slel removed this from the sage-9.3 milestone Apr 1, 2021
@slel
Copy link
Member

slel commented May 22, 2021

comment:22

Branch improving the pexpect interface to Singular now at #31846.

@slel
Copy link
Member

slel commented May 22, 2021

Changed dependencies from #31474 to none

@orlitzky
Copy link
Contributor

orlitzky commented Oct 4, 2021

Reviewer: Michael Orlitzky

@orlitzky
Copy link
Contributor

orlitzky commented Oct 4, 2021

comment:23

Addressed in #31474 and #31846.

@mkoeppe mkoeppe closed this as completed Oct 4, 2021
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
This essentially reverts 85f65bf and a10d19d from trac sagemath#31846.
It turns out this was originaly written for sagemath#30945, but that issue was
fixed by upgrading cysignals.

Singular really needs a custom `_send_interrupt()` method, because the
default one will quit singular. Moreover, this handles two quirks of
singular:

 - a small delay before sending `chr(3)` works around a bug in singular.
 - sometimes one needs to send `;` a few times after interrupt to get
   back a prompt.

The original author of the custom `_send_interrupt()` is Jeroen Demeyer
in commit 17d23e9 (trac sagemath#10476). I changed the timeout for a smaller
one, and rewrote the doctest to call `interrupt()` explicitly instead of
using `alarm()` which introduces more noise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants