Valgrind reported memory access bugs #9833
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed off by: Nick Owens - nick.owens@eero.com if we couldn't set NETLINK_EXT_ACK, there's no extra nlmsg attributes in the error. avoid UB and walking uninitialized memory by checking the flag for those attributes. for us, this avoids segfaults and in one instance, an infinite loop while walking the non-existant attributes.
Signed-off by: Nick Owens nick.owens@eero.com this removes a valgrind warning about use of uninitialized memory in a syscall when backtrace is enabled.
Size Report of OpenThread
|
abtink
reviewed
Feb 6, 2024
src/core/thread/key_manager.hpp
Outdated
| @@ -85,7 +85,7 @@ class SecurityPolicy : public otSecurityPolicy, public Equatable<SecurityPolicy> | |||
| * and Security Policy Flags. | |||
| * | |||
| */ | |||
| SecurityPolicy(void) { SetToDefault(); } | |||
| SecurityPolicy(void) : otSecurityPolicy() { SetToDefault(); } | |||
There was a problem hiding this comment.
This may not do what we intend in all cases (all toochains) since otSecuretyPolicy() is a C struct definition (so no constructor is defined for it). Default constructor may not clear all its fields (behavior can be toolchain dependent)
It may be safer to explicitly clear all its bytes. Here is my suggestion:
- We can add
public Clearable<SecurityPolicy>as a base class toclass SecurityPolicy:
class SecurityPolicy : public otSecurityPolicy, public Equatable<SecurityPolicy>, public Clearable<SecurityPolicy>
- And then we can add a
Clear()call inSetToDefault()method
There was a problem hiding this comment.
@abtink applied. let me know if that looks right
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9833 +/- ##
==========================================
+ Coverage 78.86% 82.28% +3.42%
==========================================
Files 537 555 +18
Lines 67706 73691 +5985
==========================================
+ Hits 53393 60635 +7242
+ Misses 14313 13056 -1257
|
gabekassel
force-pushed
the
gkass/netlink-memory-access
branch
from
69b695d
to
f4482ea
Compare
Signed-off by: Nick Owens nick.owens@eero.com valgrind reports that otSecurityPolicy is used uninitialized, so just make it zero. clear all bytes when setting to default make pretty
gabekassel
force-pushed
the
gkass/netlink-memory-access
branch
from
f4482ea
to
18fd2a3
Compare
abtink
reviewed
Mar 4, 2024
Co-authored-by: Abtin Keshavarzian <abtink@google.com>
Co-authored-by: Abtin Keshavarzian <abtink@google.com>
…penthread into gkass/netlink-memory-access
updated |
|
make pretty still fails - not sure why exactly |
jwhui
reviewed
Mar 5, 2024
Comment on lines
66
to
+68
| mRotationTime = kDefaultKeyRotationTime; | ||
| SetToDefaultFlags(); | ||
|
|
There was a problem hiding this comment.
Remove extra line to satisfy pretty check.
Suggested change
| mRotationTime = kDefaultKeyRotationTime; | |
| SetToDefaultFlags(); | |
| mRotationTime = kDefaultKeyRotationTime; | |
| SetToDefaultFlags(); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix one serious and two less serious uninitialized memory access bugs in openthread. All of these were found using valgrind.