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

Minor fix found in heap.c #75

Merged
merged 2 commits into from
Apr 18, 2021
Merged

Minor fix found in heap.c #75

merged 2 commits into from
Apr 18, 2021

Conversation

xiaoxiang781216
Copy link
Contributor

Fix heap_extend ignore the last block in INTERNAL_MEMORY_SPACE case
Fix the memory leak in case heap_extend return fail

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: I02fcf82131876e402f6645ff36ff06425de54744
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: I78213feec0e698533ed2234758d82f3c2bf55d67
@@ -76,7 +76,7 @@ static void *heap_extend(const int incr)
static char *bounds_p = block_o_bytes + sizeof(block_o_bytes);
static char *block_p = block_o_bytes;

if (block_p + incr >= bounds_p) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is right. If we use up to bounds_p then there was enough space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, here is an extreme example suppose INTERNAL_MEMORY_SPACE and incr both are 4KB. Then the first call heap_extend will fail due to block_p + 4KB == bounds_p even there is still 4KB free memory in the pool.

Copy link
Owner

Choose a reason for hiding this comment

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

But if block_p + 4096 is == to bounds_p then it 's going to return SBRK_ERROR right? Am I missing something? I think it should return SBRK_ERROR if it is > bounds_p only.

Copy link
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Apr 18, 2021

Choose a reason for hiding this comment

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

Yes, my change do what you describe:
The old condition(block_p + incr >= bounds_p) return SBRK_ERROR, the new condtition(block_p + incr > bounds_p) return block_p in this case.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh for goodness sake. Sorry. I see it now.

@@ -214,6 +214,7 @@ void *_dmalloc_heap_alloc(const unsigned int size)
/* shift the heap a bit to account for non block alignment */
heap_diff = heap_extend(diff_size);
if (heap_diff == SBRK_ERROR) {
heap_release(heap_new, size);
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah this makes sense.

@j256 j256 merged commit fdb5be2 into j256:master Apr 18, 2021
@xiaoxiang781216 xiaoxiang781216 deleted the heap branch April 18, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants