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

storage: fix potential memory corruption and check return values #11269

Merged
merged 1 commit into from Aug 23, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

commented Aug 21, 2019

Description

The change in l. 2484 of lfs.c is not really a fix, as we can't see anything wrong with that line, but it will silence the Coverity tool warning (thanks, @JuhPuur ).

The two other changes are true code improvements.

Pull request type

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

Reviewers

@SeppoTakalo
@VeijoPesonen
@geky

Release Notes

@ciarmcom ciarmcom requested review from geky, SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Aug 21, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

What was the coverity warning for 2484?

@@ -936,7 +936,10 @@ int SPIFBlockDevice::_reset_flash_mem()
tr_error("Sending RST failed");
status = -1;
}
_is_mem_ready();
if (false == _is_mem_ready()) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 21, 2019

Contributor

Style barf (backwards and silly boolean comparison instead of just !), but I guess it's consistent with other code nearby.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Aug 21, 2019

Author Contributor

Coverity has this nice section which I used to just keep the code consistent:

Examples where return value from this function is checked
--
A1 example_checked | SPIFBlockDevice.cpp:982 
B1 example_checked | SPIFBlockDevice.cpp:182 
C1 example_checked | SPIFBlockDevice.cpp:410 
D1 example_checked | SPIFBlockDevice.cpp:324
@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

What was the coverity warning for 2484?

Link to original warning:

incr: Incrementing cwd@dim2. The value of cwd@dim2 is now 1.
incr: Incrementing cwd@dim2. The value of cwd@dim2 is now 2.
const: At condition cwd@dim2 < 2U, the value of cwd@dim2 must be equal to 2.
dead_error_condition: The condition cwd@dim2 < 2U cannot be true.    	
CID 267716 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this statement: cwd.d.tail[cwd@dim2] = 0UL;.
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Hmm, that's just nonsense, isn't it? It's generating a warning about some sort of comparison it's generated internally while inspecting an initialiser. Weird. Still, modified version is neater.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@kjbracey-arm , I agree and I also consulted three other colleagues who had the same opinion. @JuhPuur came up with this work around we have under review here.

I think it would make sense to report this to Coverity developers for further clarification. Maybe we are missing something here? @OPpuolitaival, would you advise us if this reasonable and how to go about this?

@geky

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Change is a noop for littlefs 👍

I couldn't make heads or tails of the Coverity warning. The modified version is neater but can only be used in initialization blocks. There's a lot of pair updates in the code where you can use this shortcut which is why pair[0]=1; pair[1]=1; is used often.

@geky
geky approved these changes Aug 21, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Ci started meanwhile reviews needed (storage team) (I triggered 2 jobs, not certain how but one I aborted just now)

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 22, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit b005bf2 into ARMmbed:master Aug 23, 2019
25 checks passed
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(+0 bytes) RAM(-24 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 8722 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
@michalpasztamobica michalpasztamobica deleted the michalpasztamobica:coverity_fixes branch Aug 26, 2019
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.