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

MPR#7815: major GC crash with first-fit policy #1896

Merged
merged 2 commits into from Jul 19, 2018

Conversation

Projects
None yet
4 participants
@damiendoligez
Copy link
Member

commented Jul 11, 2018

No description provided.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

From the terseness of the description, I gather you're very sure about the fix. Fine. Would you please add a Changes entry?

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

The fix is embarassingly obvious. @stedolan, would you review?

@damiendoligez damiendoligez force-pushed the damiendoligez:fix-mpr7815 branch from f3e4edc to 6813828 Jul 12, 2018

@alainfrisch alainfrisch added the bug label Jul 12, 2018

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

This clearly fixes the buffer overflow, but I'm afraid I don't understand this code well enough to be able to give a less superficial review!

In particular, in the new code the loop can now exit having found neither the flp[i+1] block, nor a block of size at least oldsz, and I don't understand the invariants on flp well enough to know whether that's OK.

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

flp is just a fixed-size cache of blocks in order of increasing size. We are inserting a new subsequence "between" i and i+1. If the subsequence happens to fill up the remaining space, then everything from i+1 becomes irrelevant.

If you carefully follow the logic in lines 357--375 you'll see that we don't need to find more than FLP_MAX - i elements in any case.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

flp is just a fixed-size cache of blocks in order of increasing size. We are inserting a new subsequence "between" i and i+1. If the subsequence happens to fill up the remaining space, then everything from i+1 becomes irrelevant.

Got it, thanks.

If you carefully follow the logic in lines 357--375 you'll see that we don't need to find more than FLP_MAX - i elements in any case.

If I understand this correctly, we're updating flp by replacing the element at index i with a sequence of elements of length j, so the new flp should consist of:

  • i elements flp[0]..flp[i-1], then
  • j elements buf[0]..buf[j-1], then
  • flp_size - i - 1 elements flp[i+1]..flp[flp_size-1]

truncated to at most FLP_MAX elements.

I read through the logic on lines 357--375, and I think it can be simplified slightly because of the new loop condition. Since the new condition includes j < FLP_MAX - i, and since j increases by at most 1 per iteration, we have i + j <= FLP_MAX afterwards. Line 364 contains a branch on i + j < FLP_MAX, but I think both sides of the if do the same thing when i + j == FLP_MAX, so the else clause can be deleted.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

I think this PR should be merged as-is, since it fixes a nasty bug.

When reviewing, I found the logic afterwards a bit hard to follow. I think it can be simplified, but that's a separate issue. I'll make a PR for the simplification after this is merged.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Wise words from @stedolan. I'll merge ASAP.

@xavierleroy xavierleroy merged commit 802ebbf into ocaml:trunk Jul 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

xavierleroy added a commit that referenced this pull request Jul 19, 2018

MPR#7815: major GC crash with first-fit policy (#1896)
A fixed-size cache was overflowing.  This is the simple fix.
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Cherry-picked on 4.07 maintenance branch, commit 1bea41f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.