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

Segfault doing ./gap -A -c 'FORCE_QUIT_GAP(TestPackage("aclib"));' #4820

Closed
fingolfin opened this issue Mar 9, 2022 · 4 comments · Fixed by #4823
Closed

Segfault doing ./gap -A -c 'FORCE_QUIT_GAP(TestPackage("aclib"));' #4820

fingolfin opened this issue Mar 9, 2022 · 4 comments · Fixed by #4823
Labels
kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug Issues describing general bugs, and PRs fixing them

Comments

@fingolfin
Copy link
Member

fingolfin commented Mar 9, 2022

This "works" for any package, so this is a bug in GAP, not the fault of aclib, which just happened to be the example I last tried this with.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) labels Mar 9, 2022
@ChrisJefferson
Copy link
Contributor

Simplifying, the same kind of issue arises with gap -c 'Print(TestPackage("aclib"));', so it's not FORCE_QUIT_GAP's fault.

The bug is also caused by: gap -c 'Print(QuitGap(1));', so I'm fairly sure the problem is the QuitGap handling.

@ChrisJefferson
Copy link
Contributor

This has been a bug ever since the -c flag was introduced

@ChrisJefferson
Copy link
Contributor

I'm trying to trace this bug down. In general the problem is we are longjmping to a no-longer valid buffer. I'm going to brain-dump a bit below.

The pattern is the following:

QuitGap throws, which returns to some part of parsing, using TRY_IF_NO_ERROR. TRY_IF_NO_ERROR doesn't save the old value of ReadJmpError, so it is now invalid, until set again.

We carry on reading (as we don't return immediately). As we are reading from an InputTextStream, we call a GAP function to get more input from our InputTextStream (to check if it ends double-colon). This calls EvalOrExecCall, which then checks UserHasQuit, which calls GAP_THROW. However, since the last GAP_THROW we haven't updated the jump buffer.

So in short the problem is:

  • We call QuitGap.
  • Because QuitGap exits GAP 'nicely', we try to read until the end of the current statement.
  • That requires we read some more characters from our input stream
  • IntrFuncCallEnd tries to "exit early" from that call.
  • We haven't done a setjmp, so bad things happen.

So, some fix choices are (and I'm curious what @fingolfin thinks, as I think these might involve retangling previously untangled things).

  1. Make the reader not try reading more characters if UserHasQUIT.
  2. Temporarily disable UserHasQUIT while we are reading new text, as it is "vital".
  3. Make UserHasQUIT exit more eagerly, but without using GAP_THROW.

We could also do these things above if we have NrError > 0.

@ChrisJefferson
Copy link
Contributor

Further thoughts, I think GAP normally assumes once a GAP_THROW has been performed, we won't evaluate any more GAP functions until the end of the current statement, but of course that's not true if we are reading from an InputTextString and we need more input.

This isn't just a "quitting" problem, for example READ_NORECOVERY(InputTextString("Print(1 + [()]);")); segfaults as well once we exit the break loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants