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

littlefs: Fixed issue with cleanup in mount function on error #7851

Merged
merged 2 commits into from Aug 23, 2018

Conversation

@geky
Member

geky commented Aug 21, 2018

Description

As a part of the v1.6 update (#7713), littlefs added proper handling for cleaning up memory in the case of an error during mount (ARMmbed/littlefs#80). This took care of a memory leak users were seeing. Ironically, it turns out the implementation and user patterns in mbed-os was relying on this memory leak to avoid a double free in the same case of an error during mount.

The issue was that a failed mount would leave the LittleFileSystem class in a state where it thought it was mounted, and later it would attempt to unmount the filesystem. With the previous memory leak this would be "ok", and the leaked memory would be freed. But with the fix in v1.6, no memory is leaked, and the incorrect free triggers a hard fault.

Fixed to clean up state properly on failed mounts.

cc @juhoeskeli, @dannybenor, @deepikabhavnani, @ARMmbed/mbed-os-storage

Pull request type

[ ] Fix
[x] Fix for unreleased feature
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

geky added some commits Aug 21, 2018

littlefs: Fixed issue with cleanup in mount function on error
As a part of the v1.6 update, littlefs added proper handling for
cleaning up memory in the case of an error during mount. This took care
of a memory leak users were seeing. Ironically, it turns out the implementation
and user patterns in mbed-os was _relying_ on this memory leak to avoid a
double free in the same case of an error during mount.

The issue was that a failed mount would leave the LittleFileSystem class in a
state where it thought it was mounted, and later it would attempt to
unmount the filesystem. With the previous memory leak this would be
"ok", and the leaked memory would be freed. But with the fix in v1.6,
no memory is leaked, and the incorrect free triggers a hard fault.

Fixed to clean up state properly on failed mounts.

@cmonr cmonr requested a review from ARMmbed/mbed-os-storage Aug 22, 2018

@cmonr cmonr added the needs: review label Aug 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 22, 2018

[x] Fix dependent on unreleased feature

Well that's a new one.

@juhoeskeli

Looks good to me. Tested the PR and it fixes the problem I was having.

@davidsaada

Looks good to me.

@OPpuolitaival

Looks good

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 22, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 22, 2018

Build : SUCCESS

Build number : 2861
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7851/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 23, 2018

@0xc0170 0xc0170 merged commit 603c4f9 into ARMmbed:master Aug 23, 2018

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 585 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9171 cycles (-1316 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@mbed-ci

This comment has been minimized.

LFS_INFO("unmount -> %d", lfs_toerror(err));
_mutex.unlock();
return lfs_toerror(err);
if (err && !res) {

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 23, 2018

Contributor

Just a nit: !res check is not really needed here Too late :-)

This comment has been minimized.

@geky

geky Aug 23, 2018

Member

Yeah, I just kept it for consistency. That way if any code is added between the declaration of res and this line, it won't break anything.

The compiler will optimize it out anyways.

This comment has been minimized.

@cmonr

cmonr Aug 23, 2018

Contributor

The compiler will optimize it out anyways.

Famous last words

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 29, 2018

Merge pull request ARMmbed#7851 from geky/fix-littlefs-mount-cleanup
littlefs: Fixed issue with cleanup in mount function on error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment