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

Crash in coder/executor with foo!{[1,2,3]} syntax #2765

Closed
fingolfin opened this issue Aug 31, 2018 · 0 comments · Fixed by #2766
Closed

Crash in coder/executor with foo!{[1,2,3]} syntax #2765

fingolfin opened this issue Aug 31, 2018 · 0 comments · Fixed by #2766
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 kind: discussion discussions, questions, requests for comments, and so on

Comments

@fingolfin
Copy link
Member

Consider this:

gap> f := function(l) l!{[1,3]} := [42, 23]; end;;
gap> Display(f);
Error, Panic: cannot print statement of type '55' in
  <corrupted statement>  called from
...
gap> f([]);
Panic: tried to execute a statement of unknown type '55'

With some more effort, it can even lead to a crash:

gap> function()
>   local l;
>   l:=[1,2,3];
>   Display(l);
>   l![1] := 42;
>   Display(l![1]);
>   Display(IsBound(l![1]));
>   Unbind(l![1]);
>   Display(l);
>   l!{[1,3]} := [42, 23];
>   Display(l);
>   Display(l!{[3,1]});
> end();
[ 1, 2, 3 ]
42
true
[ , 2, 3 ]
Panic: tried to execute a statement of unknown type '55'
[ , 2, 3 ]
Panic: tried to evaluate an expression of unknown type '189'
Segmentation fault: 11

What's going on here? Well, that's because support for list slices via the list{indices} syntax was not fully extended to its "bang-variant", list!{indices}, which is for accessing slices of positional objects.

In the interpreter, we thus just print an error: "sorry, not supported". But when coding, the coder happily codes this. But printing and executing was never implemented for these kinds of statements, hence the weird errors and ultimately the crash. Specifically, at least these six statement/expression types are affected:

T_ASSS_POSOBJ
T_ASS_POSOBJ_LEV
T_ASSS_POSOBJ_LEV
T_ELMS_POSOBJ
T_ELM_POSOBJ_LEV
T_ELMS_POSOBJ_LEV

Now, we could implement printing and coding for them, in a minimalistic fashion. But I wonder if it is worth it -- do we really want to support !{...}? As far as I can tell, it's not documented. So I would rather prefer to remove it completely -- most of the existing support code ends up printing "sorry, not yet implemented" anyway.

Or perhaps somebody can think of a reason to keep it?

@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) kind: request for comments labels Aug 31, 2018
fingolfin added a commit to fingolfin/gap that referenced this issue Aug 31, 2018
Specifically, `posobj!{indices}` was meant to relate to `list{indices}` the
same way as `posobj![i]` does to `list[i]`. But it was never fully
implemented. Specifically, no printing or execution functions for the
relevant statement and expression types were implemented; and for the
interpreter and compiler, mostly trying to use these lead to an error a long
the line of "sorry, this feature has not been implemented". Due to all this,
trying to use this feature in a coded function could trigger all kinds of
problems, up to crashes.

Since this code serves no clear purpose, we just remote it, instead of trying
to implement the missing bits.

Fixes gap-system#2765
fingolfin added a commit to fingolfin/gap that referenced this issue Aug 31, 2018
Specifically, `posobj!{indices}` was meant to relate to `list{indices}` the
same way as `posobj![i]` does to `list[i]`. But it was never fully
implemented. Specifically, no printing or execution functions for the
relevant statement and expression types were implemented; and for the
interpreter and compiler, mostly trying to use these lead to an error a long
the line of "sorry, this feature has not been implemented". Due to all this,
trying to use this feature in a coded function could trigger all kinds of
problems, up to crashes.

Since this code serves no clear purpose, we just remote it, instead of trying
to implement the missing bits.

Fixes gap-system#2765
fingolfin added a commit to fingolfin/gap that referenced this issue Sep 2, 2018
Specifically, `posobj!{indices}` was meant to relate to `list{indices}` the
same way as `posobj![i]` does to `list[i]`. But it was never fully
implemented. Specifically, no printing or execution functions for the
relevant statement and expression types were implemented; and for the
interpreter and compiler, mostly trying to use these lead to an error a long
the line of "sorry, this feature has not been implemented". Due to all this,
trying to use this feature in a coded function could trigger all kinds of
problems, up to crashes.

Since this code serves no clear purpose, we just remote it, instead of trying
to implement the missing bits.

Fixes gap-system#2765
fingolfin added a commit that referenced this issue Sep 4, 2018
Specifically, `posobj!{indices}` was meant to relate to `list{indices}` the
same way as `posobj![i]` does to `list[i]`. But it was never fully
implemented. Specifically, no printing or execution functions for the
relevant statement and expression types were implemented; and for the
interpreter and compiler, mostly trying to use these lead to an error a long
the line of "sorry, this feature has not been implemented". Due to all this,
trying to use this feature in a coded function could trigger all kinds of
problems, up to crashes.

Since this code serves no clear purpose, we just remote it, instead of trying
to implement the missing bits.

Fixes #2765
@fingolfin fingolfin added kind: discussion discussions, questions, requests for comments, and so on and removed kind: question labels Mar 21, 2019
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 kind: discussion discussions, questions, requests for comments, and so on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant