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

Clean up ex-malloc() use of CHIPMem.h #3222

Merged

Conversation

kpschoedel
Copy link
Contributor

Problem

As part of heap memory management, a recent change (#3143) replaced
malloc()-family calls one-for-one with their "CHIPMem.h" equivalents,
but a one-to-one replacement is not always ideal.

Summary of Changes

  • Use ScopedMemoryBuffer instead where possible.
  • Added ScopedMemoryString, since this is a common case.
  • Some additional safety checks.

#### Problem

As part of heap memory management, a recent change (project-chip#3143) replaced
malloc()-family calls one-for-one with their "CHIPMem.h" equivalents,
but a one-to-one replacement is not always ideal.

#### Summary of Changes

- Use ScopedMemoryBuffer instead where possible.
- Added ScopedMemoryString, since this is a common case.
- Some addition safety checks.
src/lib/core/tests/TestCHIPTLV.cpp Outdated Show resolved Hide resolved
{
if (dest)
{
memcpy(dest, source, length);
Copy link
Contributor

Choose a reason for hiding this comment

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

any issues if source is shorter than length? Maybe we could use strncpy internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to say no (NUL in the source is fine, NUL not in the source is fine) but there is one case where memcpy would break (NUL-terminated source closely followed by dangerous addresses).

@BroderickCarlin BroderickCarlin merged commit 61215e5 into project-chip:master Oct 16, 2020
@kpschoedel kpschoedel deleted the x2899-mem-malloc-followup branch October 23, 2020 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants