lib/os/mempool: Fix corruption case with block splitting #16966
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewboie
approved these changes
Jun 20, 2019
lib/os/mempool.c
Outdated
Contributor
There was a problem hiding this comment.
suggest parens around lvl == 0 for clarity.
same deal with following line.
or just use a regular if { } else { }. I had to blink at this a lot to correctly parse it.
andyross
force-pushed
the
mempool-block-fits
branch
from
fe4621e to
fff4f07
Compare
Contributor
Author
|
Spread out formatting for clarity |
Contributor
Author
|
One more tiny patch to address a buffer allocation bug @npitre pointed out (who I still can't seem to add as a reviewer, but who can surely just jump in). |
andyross
force-pushed
the
mempool-block-fits
branch
from
e7a94d9 to
b36f732
Compare
|
On Thu, 20 Jun 2019, Andy Ross wrote:
One more tiny patch to address a buffer allocation bug @npitre pointed out (who I still can't seem to add as a reviewer, but who can surely just jump in).
I agree with the first patch, the block_fits() one, for now at least.
I also agreed with the second patch in principle. Now that I've
actually seen it I must retract that.
Your patch is not sufficient. You missed one header file. But more
importantly, you are missing some small but again fundamental details.
Please consider #16996 where I provided the complete explanation.
|
The block_fits() predicate was borked. It would check that a block fits within the bounds of the whole heap. But that's not enough: because of alignment changes between levels the sub-blocks may be adjusted forward. It needs to fit inside the PARENT block that it was split from. What could happen at runtime is that the last subblocks of a misaligned parent block would overlap memory from subsequent blocks, or even run off the end of the heap. That's bad. Change the API of block_fits() a little so it can extract the parent region and do this properly. Fixes zephyrproject-rtos#15279. Passes test introduced in zephyrproject-rtos#16728 to demonstrate what seems like the same issue. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
andyross
force-pushed
the
mempool-block-fits
branch
from
b36f732 to
59c39b5
Compare
Contributor
Author
|
Yank duplicated fix. Rebase. |
|
In case my opinion is still required, I approve this PR as it is.
Please someone merge this ASAP. The potential for weird bugs is real without it.
|
npitre
pushed a commit
to npitre/zephyr
that referenced
this pull request
Jun 26, 2019
This test demonstrates a serious bugs that exist in the mem-pool code as ovf Zephyr v1.14.0, and probably earlier versions too. This bug is fixed with PR zephyrproject-rtos#16966. Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The block_fits() predicate was borked. It would check that a block
fits within the bounds of the whole heap. But that's not enough:
because of alignment changes between levels the sub-blocks may be
adjusted forward. It needs to fit inside the PARENT block that it was
split from.
What could happen at runtime is that the last subblocks of a
misaligned parent block would overlap memory from subsequent blocks,
or even run off the end of the heap. That's bad.
Change the API of block_fits() a little so it can extract the parent
region and do this properly.
Fixes #15279. Passes test introduced in #16728 to demonstrate what
seems like the same issue.
Signed-off-by: Andy Ross andrew.j.ross@intel.com