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

nimble/host: Zero initialize buffer after allocation #1687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rahult-github
Copy link
Contributor

At many places , after getting buffer using os_memblock_get, buffer is not zero initiatlized. Added change to memset the same to 0.

@rahult-github rahult-github force-pushed the bugfix/memset_allocated_mem branch 2 times, most recently from 6b954aa to e9be212 Compare February 13, 2024 16:38
@rahult-github
Copy link
Contributor Author

@andrzej-kaczmarek , @sjanc , please take a look

@sjanc
Copy link
Contributor

sjanc commented Mar 8, 2024

Hi,

Does this fix any use of uninitialized memory or is just a precaution?

As for style, please use sizeof(*foo) syntax

@rahult-github
Copy link
Contributor Author

Hi,

Does this fix any use of uninitialized memory or is just a precaution?

As for style, please use sizeof(*foo) syntax

Currently , just precaution, since i didn't find any issue as such during my testing. But in many places in nimble, such memset is already existing, hence added at places where it is missing.

For style, sure, will change and re-push

@rahult-github
Copy link
Contributor Author

Hello @sjanc , please take a look

Copy link
Contributor

@sjanc sjanc left a comment

Choose a reason for hiding this comment

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

I'm not sure about this patch, most are memseting places that already initialize all fields afterwards, and some of them are frequently called, add extra overhead o function call for no little benefit

maybe we should memset (or explicitly initialized members) only where it is really needed?

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.

2 participants