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, as that leads to crashes #4544

Merged
merged 1 commit into from
Aug 28, 2021

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jun 7, 2021

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.

(This used to be PR #4349 but I accidentally deleted the underlying branch, it seems, so I had to reopen it in a fresh PR)

@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 Jun 7, 2021
@fingolfin fingolfin requested a review from wilfwilson June 7, 2021 17:42
@fingolfin
Copy link
Member Author

The problem is that irredsol 1.4.2 does not actually contain the fix; I made a corresponding comment at bh11/irredsol#12

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change will be needed for the tests to pass (eventually).

tst/testbugfix/2021-03-25-InstallMethod.tst Show resolved Hide resolved
@wilfwilson wilfwilson added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jun 10, 2021
@wilfwilson
Copy link
Member

From a quick look at the code, it seems that irredsol 1.4.3 has the right fix, so hopefully this can be merged soon.

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
Copy link
Member Author

@alex-konovalov to make progress with this, we need irredsol 1.4.3 in the package distribution. But https://github.com/gap-system/gap/wiki/Package-updates-status was last updated in May...

@olexandr-konovalov
Copy link
Member

Done - they were as on 2nd June, now as of today. Updated tables of contents at https://github.com/gap-system/gap-distribution/releases/tag/package-archives and https://github.com/gap-system/gap/wiki/Package-updates-status

@olexandr-konovalov
Copy link
Member

Seems like CI tests are passing, except for some unrelated quirks.

@fingolfin fingolfin closed this Aug 27, 2021
@fingolfin fingolfin reopened this Aug 27, 2021
@wilfwilson wilfwilson merged commit 4a9e6fd into gap-system:master Aug 28, 2021
@fingolfin fingolfin deleted the mh/dont-InstallMethod-self branch August 28, 2021 20:22
@fingolfin fingolfin changed the title Forbid installing an operation as method for itself Forbid installing an operation as method for itself, as that leads to crashes Aug 17, 2022
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.

None yet

3 participants