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 buffer overflow in TransformationListListNC #3463

Merged
merged 1 commit into from
May 21, 2019

Conversation

ChrisJefferson
Copy link
Contributor

Fix a buffer overflow in TransformationListList. This bug is in 4.10 (might be in earlier releases too)

Text for release notes

  • Fix bug in TransformationListList which could cause a crash.

@ChrisJefferson ChrisJefferson added kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) topic: kernel release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes backport-to-4.10 labels May 18, 2019
@ChrisJefferson
Copy link
Contributor Author

@james-d-mitchell : I realise this is a busy time of year, just wanted to link you to this in case you had any opinion about it / I'd done anything stupid.

@coveralls
Copy link

coveralls commented May 18, 2019

Coverage Status

Coverage remained the same at 85.169% when pulling da7909c on ChrisJefferson:fix-trans into 8e281e4 on gap-system:master.

@codecov
Copy link

codecov bot commented May 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@8e281e4). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #3463   +/-   ##
=========================================
  Coverage          ?   85.34%           
=========================================
  Files             ?      699           
  Lines             ?   346475           
  Branches          ?        0           
=========================================
  Hits              ?   295685           
  Misses            ?    50790           
  Partials          ?        0
Impacted Files Coverage Δ
src/trans.cc 99.8% <100%> (ø)

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.

Two minor code formatting changes, please.
More importantly, I don't understand why this fixes a buffer overflow. It seems that this change just avoids an unnecessary assignment. So clearly I am missing something. Would you mind giving me a hint? Unless it is super trivial in the end, perhaps there should be a code comment?

src/trans.cc Outdated Show resolved Hide resolved
src/trans.cc Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

woops, done some clang-formatting.

The problem (I've tweaked the description to hopefully help) is that at the beginning of the function when we figure out the size of the transformation we need, we basically take the max of the values in the two arrays, EXCEPT we skip any index i where the two lists are equal. This makes sense, because if the transformation maps x to x for any x, we don't need to explicitly store that fact, as that's the default for transformations.

Except, if (say) the original arrays were [[1,1000], [2,1000]], we would decide we need a transformation of size 2, but then later try to fill in that the transformation maps 1000 to 1000, just writing into the memory past the end of the transformation.

This almost always won't be into another allocated bag (because we've only just allocated this memory), but it will make some future allocated bag be allocated with non-zero values, which is (in practice) basically as bad.

To save memory, when calculating the size of the transformation,
indices where src[i]=ran[i] are skipped. Therefore these same
indices must be skipped skipped when filling in the transformation,
as we have not allocated memory for them.
@ChrisJefferson
Copy link
Contributor Author

also chucked a comment in

@james-d-mitchell
Copy link
Contributor

This makes sense to me @ChrisJefferson, well spotted!

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.

Thank you, that explanation makes perfect sense.

@ChrisJefferson ChrisJefferson merged commit ecb201b into gap-system:master May 21, 2019
@ChrisJefferson ChrisJefferson deleted the fix-trans branch May 21, 2019 08:20
@fingolfin fingolfin mentioned this pull request Jun 4, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.2 milestone Jun 12, 2019
@fingolfin fingolfin modified the milestone: GAP 4.10.2 Jun 13, 2019
@olexandr-konovalov olexandr-konovalov 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 Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.10-DONE kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants