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

fs: fs_open can corrupt fs_open_t object given via zfp parameter #29478

Closed
de-nordic opened this issue Oct 23, 2020 · 1 comment · Fixed by #31597
Closed

fs: fs_open can corrupt fs_open_t object given via zfp parameter #29478

de-nordic opened this issue Oct 23, 2020 · 1 comment · Fixed by #31597
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@de-nordic
Copy link
Collaborator

Describe the bug
There is no check, within fs_open, whether the provided zfp already points to used object.
This means that when fs_open is used twice on the same object it will overwrite contents of
zfp before passing execution to file system driver.
This may lead to two possible outcomes, depending on whether zfp is used for opening (1) file with the same path or (2) file with different path.
According to fs_api tests (1) should end with an error, but this is actually left to file system driver and is not certain.
In both cases the file system driver will be called which may result in resource leak; e.g. LittleFS will allocate slab memory overwriting original pointer within provided fs_file_t object before further deciding if open is successful or not; this means that at this point, if there has been already memory allocated by previous fs_open, if such occurred.
Depending on whether error occurred or not, and on stage, the pointers may be pointing to new resources, that might have been already released and is invalid, or NULL

To Reproduce
Using LittleFS mounted partition.
Call fs_open, on different or the same file, twice and observe fs_file_t->filep.

Expected behavior
Not breaking the original structure and returning with error instead.

Impact
Possible data corruption, resource leaks, double assignment of the same file system driver structures to different file streams.

Workaround
Always fs_close fs_file_t before reusing structure. Avoid double-open as even on error the original fs_file_t object will be damaged.

Environment (please complete the following information):
-- zephyrproject-rtos/zephyr: c13603e

@de-nordic de-nordic added the bug The issue is a bug, or the PR is fixing a bug label Oct 23, 2020
@nashif nashif added the priority: medium Medium impact/importance bug label Oct 27, 2020
@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Dec 27, 2020
@nvlsianpu nvlsianpu removed the Stale label Dec 31, 2020
de-nordic added a commit to de-nordic/zephyr that referenced this issue Jan 28, 2021
Fixes problem when fs_open invoked on fs_file_t object, which is already
holding information on opened file, overwrites references to other
memory objects within the fs_file_t object causing resource leak.
If fs_open is invoked on already used fs_file_t object, it will return
-EBUSY.

Note: The change requires that all fs_file_t objects should be
initialized to 0.

Fixes: zephyrproject-rtos#29478

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
@nvlsianpu nvlsianpu removed their assignment Jan 29, 2021
nashif pushed a commit that referenced this issue Jan 29, 2021
Fixes problem when fs_open invoked on fs_file_t object, which is already
holding information on opened file, overwrites references to other
memory objects within the fs_file_t object causing resource leak.
If fs_open is invoked on already used fs_file_t object, it will return
-EBUSY.

Note: The change requires that all fs_file_t objects should be
initialized to 0.

Fixes: #29478

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants