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

Bring the heap_4 improvements to secure_heap #749

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

Moral-Hao
Copy link
Contributor

@Moral-Hao Moral-Hao commented Aug 12, 2023

Description

This includes improvements like addition overflow checks, readability improvements.

Test Steps

Tested locally.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

It is possible the modified wanted size is too large,
we should check it again.
@Moral-Hao Moral-Hao requested a review from a team as a code owner August 12, 2023 12:39
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (bcd6dbd) 94.35% compared to head (c082649) 94.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #749   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files           6        6           
  Lines        2443     2443           
  Branches      598      598           
=======================================
  Hits         2305     2305           
  Misses         85       85           
  Partials       53       53           
Flag Coverage Δ
unittests 94.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ydhuang28
Copy link

@Moral-Hao Thank you for the PR. The check for ( xWantedSize & xBlockAllocatedBit ) == 0 is already done by the larger if-statement that contains the if-statement that you've changed. Therefore, these changes are not necessary. I'm closing the PR, but feel free to open a new one if you find a different issue. Thanks!

@ydhuang28 ydhuang28 closed this Aug 14, 2023
@Moral-Hao
Copy link
Contributor Author

Moral-Hao commented Aug 15, 2023

@ydhuang28 I don't agree with you. The larger if-statement is checking the original xWantedSize, and variable xWantedSize is changed after that (it will add space for heap structure and for alignment padding).
It is possible that the changed xWantedSize is large enough to make xBlockAllocatedBit being set in xWantedSize. We should check it, and that why I call this PR as check again.

@ydhuang28 ydhuang28 reopened this Aug 15, 2023
Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
@aggarg aggarg changed the title Check the wanted size again after modify it. Bring the heap_4 improvements to secure_heap Aug 16, 2023
@Moral-Hao
Copy link
Contributor Author

@aggarg Thank you, your code change is better than mine.

@aggarg aggarg merged commit bd720c3 into FreeRTOS:main Aug 16, 2023
13 checks passed
@Moral-Hao Moral-Hao deleted the check_wanted_size_again_after_modify branch August 17, 2023 01:21
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.

4 participants