Skip to content

Conversation

@fingolfin
Copy link
Member

We did not mark the forwarding pointer when copying. As a result, a badly timed GC could have reaped the copy of the wpobj, and then when we try to finalize the copying, a crash (or, if we were really unlucky, corrupt data) could have resulted.

I never observed this manifest, though; I only discovered it while trying together with @rbehrends to track down a crash with Julia GC; while inspecting our code in PR #2688, I realized that the bug fixed here exists (and that I had "accidentally" fixed it in PR #2688 for Julia GC, because I changed the code there to not use MarkWeakPointerObj anymore).

This issue is also resolved by PR #2777, which completely refactors the copying code, and essentially makes this kind of bug impossible.

We did not mark the forwarding pointer when copying.
@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) topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 6, 2018
@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #2780 into master will increase coverage by 7.77%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2780      +/-   ##
==========================================
+ Coverage    68.1%   75.87%   +7.77%     
==========================================
  Files         480      481       +1     
  Lines      241225   241312      +87     
==========================================
+ Hits       164275   183099   +18824     
+ Misses      76950    58213   -18737
Impacted Files Coverage Δ
src/weakptr.c 97.36% <50%> (-0.36%) ⬇️
hpcgap/lib/hpc/stdtasks.g 63.42% <0%> (-0.77%) ⬇️
lib/teachmod.g 12.94% <0%> (ø)
src/pperm.c 97.88% <0%> (+0.05%) ⬆️
src/vecgf2.c 72.83% <0%> (+0.05%) ⬆️
src/io.c 59.9% <0%> (+0.07%) ⬆️
grp/simple.gi 77.63% <0%> (+0.11%) ⬆️
src/gap.c 85.18% <0%> (+0.14%) ⬆️
lib/wordrep.gi 66.61% <0%> (+0.16%) ⬆️
src/exprs.c 96.41% <0%> (+0.17%) ⬆️
... and 182 more

@ChrisJefferson ChrisJefferson merged commit fe8f60c into gap-system:master Sep 6, 2018
@fingolfin fingolfin deleted the mh/fix-MarkWeakPointerObj branch September 7, 2018 07:22
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: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants