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

Forbid installing an operation as method for itself #4349

Closed

Conversation

fingolfin
Copy link
Member

Triggering such a "method" would lead to a segfault due to a stack overflow.

Of course one can still trigger this by e.g. having two operations foo and
bar, and installing fooas method forbar` and vice-versa. But that is far
less likely to happen by accident. Also, it seems better to fix at least some
instances of this issue than to give up because we can't fix it completely.

Essentially resolves #4340 and #1286

Blocked by the fact that at least one method (irredsol) actually installs an operation as method for itself, albeit by accident, and in a fashion that should make it impossible for this method to ever be actually triggered in "normal" situations. Still, this PR will cause this to throw an error, so we can't merge it until irredsol gets a new release with a fix, such as bh11/irredsol#10.

Triggering such a "method" would lead to a segfault due to a stack overflow.

Of course one can still trigger this by e.g. having two operations `foo` and
`bar, and installing `foo` as method for `bar` and vice-versa. But that is far
less likely to happen by accident. Also, it seems better to fix at least some
instances of this issue than to give up because we can't fix it completely.
@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) release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Mar 25, 2021
@wilfwilson
Copy link
Member

IRREDSOL 1.4.2 was released with your fix, will some extra step be required for it to show up in the CI?

@fingolfin
Copy link
Member Author

@wilfwilson the package distribution needs to pick it up, @alex-konovalov takes care of that

@olexandr-konovalov
Copy link
Member

IRREDSOL picked up (https://github.com/gap-system/gap/wiki/Package-updates-status) but package archives are not yet used for our CI - wait till I update https://github.com/gap-system/gap-distribution/releases/tag/package-archives

@wilfwilson
Copy link
Member

Looks like it's listed there now.

@wilfwilson
Copy link
Member

I am struggling to build this PR (because despite the CI now using IRREDSOL, it seems that something is still installing an operation as a method for itself, and I wanted to see which one - that is not obvious from the output of the CI). Could you perhaps rebase, when convenient?

This is the problem I get:

   LINK    gap
   LINK    libgap.la
libtool:   error: 'build/obj/src/c_oper1.c.lo' is not a valid libtool object
make: *** [gap] Error 1
make: *** Waiting for unfinished jobs....
libtool:   error: 'build/obj/src/c_oper1.c.lo' is not a valid libtool object
make: *** [libgap.la] Error 1

@fingolfin fingolfin closed this Jun 7, 2021
@fingolfin fingolfin deleted the mh/dont-InstallMethod-self branch June 7, 2021 17:35
@fingolfin
Copy link
Member Author

errrr... I am not sure what happened here, and why the branch was deleted and this PR closed?!? I must have screwed up something. I'll open a fresh PR (as I can't seem to restore this one)

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 release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault after installing a silly method for a synonym attribute
3 participants