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

FlashIAP::init - Possible memory leak/OOM error #12439

Closed
dustin-crossman opened this issue Feb 14, 2020 · 3 comments · Fixed by #12531
Closed

FlashIAP::init - Possible memory leak/OOM error #12439

dustin-crossman opened this issue Feb 14, 2020 · 3 comments · Fixed by #12531

Comments

@dustin-crossman
Copy link
Contributor

Description of defect

If a targets flash_init() returns an errror, FlashIAP::init() will catch this and set its own return code properly, however, it does not return early. As a result it will call get_page_size() and then use new to allocate a page.
This can lead to two problems:

  1. If new allocates successfully a memory leak may occur because the user will not expect (and should not be expected) to call deinit() after a failed init.
  2. If get_page_size() returns wrong/junk data new may be called with a very large value and cause an Out of Memory error.

Issue #2 was seen in practice while we were testing changes to our flash driver which caused it to fail on init and which therefore caused get_page_size() to return an uninitialized value.

Target(s) affected by this defect ?

All

Toolchain(s) (name and version) displaying this defect ?

All

What version of Mbed-os are you using (tag or sha) ?

master
sha: 3d038e5

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

Modify a targets flash_init() to return failure.

@ciarmcom
Copy link
Member

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2549

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 17, 2020

Thanks for the report, could you propose a fix and send a pull request?

If we fail in init, we should not allocate anything and return an error. It would fix 1. and also 2. (we don't have there yet defined behavior but should be call deinit only on initialized flash otherwise undefined behavior - something similar to what serial already has defined: Calling any function other than ::serial_init on am uninitialized or freed serial_t.). Proper fixing for 2nd will be once we add this behavior.

cc @ARMmbed/mbed-os-hal

@dustin-crossman
Copy link
Contributor Author

I can get a PR in for a fix. Probably won't be until next week though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants