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

Fix bug in CycleStructurePerm for a single cycle of length 2^16 that caused wrong answers and memory corruption #3738

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

ChrisJefferson
Copy link
Contributor

CYCLE_STRUCT_PERM overflowed a UInt2 when given a cycle of length
exactly 2^16, so store the size-1 (which is what we output anyway)

This caused both wrong answers, and memory corruption.

This function could, in principle, be overhauled/simplified to use less fancy buffers, but I'm convinced by this fix and wanted something small to go into 4.11 at least.

@ChrisJefferson ChrisJefferson added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, 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: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes backport-to-4.11 labels Nov 12, 2019
@coveralls
Copy link

coveralls commented Nov 12, 2019

Coverage Status

Coverage remained the same at 84.512% when pulling fefc497 on ChrisJefferson:fix-cycle-struct-perm into 79118b8 on gap-system:master.

src/permutat.cc Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me; two minor comments

tst/testinstall/kernel/permutat.tst Show resolved Hide resolved
CycleStructPerm overflowed a UInt2 when given a cycle of length
exactly 2^16, so store the size-1 (which is what we output anyway)
@ChrisJefferson
Copy link
Contributor Author

I believe all issues are now fixed

@fingolfin fingolfin merged commit 5884d14 into gap-system:master Nov 14, 2019
@fingolfin
Copy link
Member

Backported to stable-4.11 in commit 7d39a54

@ChrisJefferson ChrisJefferson deleted the fix-cycle-struct-perm branch December 2, 2019 14:30
@fingolfin fingolfin changed the title Fix bug in CycleStructurePerm for a single cycle of length 2^16 Fix bug in CycleStructurePerm for a single cycle of length 2^16 that cause wrong answers and memory corruption Dec 5, 2019
@fingolfin fingolfin changed the title Fix bug in CycleStructurePerm for a single cycle of length 2^16 that cause wrong answers and memory corruption Fix bug in CycleStructurePerm for a single cycle of length 2^16 that caused wrong answers and memory corruption Dec 5, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since 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 Dec 5, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.11-DONE kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants