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

Fix Coverity uninitialized pointer field Thread.h #11273

Merged
merged 1 commit into from Aug 22, 2019

Conversation

@Tharazi97
Copy link
Contributor

commented Aug 21, 2019

Description

Changed _obj_mem to be initialized with creating new thread.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@jamesbeyond @maciejbocianski

Release Notes

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Aug 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@Tharazi97, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@@ -540,7 +540,7 @@ class Thread : private mbed::NonCopyable<Thread> {
bool _dynamic_stack;
Semaphore _join_sem;
mutable Mutex _mutex;
mbed_rtos_storage_thread_t _obj_mem;
mbed_rtos_storage_thread_t _obj_mem = {};

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 21, 2019

Member

The current init is in start() method - it sets this to 0:

memset(&_obj_mem, 0, sizeof(_obj_mem));

Assuming based on how it is initialized (POD type), this could be handled in the constructor as the rest of members - to be consistent however this is valid now as it is.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 21, 2019

Contributor

I'm not a fan of the existing constructor method and its in-body init - now we're C++11 you could use proper constructor delegation, and the final most-flexible constructor could do everything via its member-initializer list.

I'd also prefer if everything was consistent - all defaults in body of constructor, all defaults in member-initializer-list, or all defaults as default-member initializers.

So make it a memset in constructor, or do a more wide-ranging refactor/tidy including real constructor delegation.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I guess this is warning about get_state accessing _obj_mem.state? It can't figure out that _tid only becomes non-NULL after a memset of _obj_mem? Otherwise, not sure why it would think this is a problem.

I think this is probably fine, but you should net off the size increase a bit by removing the memset from Thread::start.

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Yes I think it is warning caused by get_state(). In constructor _tid gets asigned to 0. Maybe it should have become nullptr in constructor'' and memset'' of _obj_mem should be left in start?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Yes I think it is warning caused by get_state(). In constructor _tid gets asigned to 0. Maybe it should have become nullptr in constructor'' and memset'' of _obj_mem should be left in start?

You could certainly set _tid to nullptr as a style change, but that won't be impacting Coverity's complaint (99% certainty).

Fix Coverity uninitialized pointer field Thread.h
Changed _obj_mem to be initialized with constructor of new thread.

@Tharazi97 Tharazi97 force-pushed the Tharazi97:Coverty-threads branch from b1ced1b to 6467f56 Aug 21, 2019

@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@kjbracey-arm you were right. I've moved memset to the constructor.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 21, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

started CI

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 21, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@Tharazi97

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

CI fail seems not related.
test case: 'tests-mbed_hal-reset_reason' ..................................................... FAIL in 48.35 sec

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Test restarted

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 22, 2019

@0xc0170 0xc0170 merged commit 0c18a77 into ARMmbed:master Aug 22, 2019

25 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-8 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8556 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.