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

kconfiglib.py _save_old() may rename /dev/null -- replacing /dev/null with a file #31362

Closed
domo141 opened this issue Jan 15, 2021 · 3 comments · Fixed by #31438
Closed

kconfiglib.py _save_old() may rename /dev/null -- replacing /dev/null with a file #31362

domo141 opened this issue Jan 15, 2021 · 3 comments · Fixed by #31438
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@domo141
Copy link

domo141 commented Jan 15, 2021

Describe the bug

the kconfiglib.py _save_old("/dev/null") may actually, unexpectly, succeed.

afterwards /dev/null was opened for writing as a (new) file

To Reproduce
Steps to reproduce the behavior:

  1. mkdir build; cd build
  2. cmake -DBOARD=board_xyz
  3. ls -l /dev/null
  4. cat /dev/null

Expected behavior

/dev/null is still device node

Impact

/dev/null is a file, with kconfig content

What impact does this issue have on your progress

host code that opened /dev/null as config file got "garbage"

Environment (please complete the following information):

  • a docker container (not as root, but probably enough capabilites to rename /dev/null)

zephyr v1.14 -- but after investigating main in 2021-01 it still can happen

** how to fix **

if not os.path.isfile(path) return to skip rename if path does not point to a file
(either directly, or via symlink)

@domo141 domo141 added the bug The issue is a bug, or the PR is fixing a bug label Jan 15, 2021
@nashif nashif added the priority: medium Medium impact/importance bug label Jan 19, 2021
@nashif
Copy link
Member

nashif commented Jan 19, 2021

@domo141 Can you please submit a PR with the proposed fix?

@domo141
Copy link
Author

domo141 commented Jan 19, 2021

Created (as usual, change was simple, comments and commit message hard)...

#31438: based on commit msgs ulfalizer may (or may not) follow up soon...

@nashif
Copy link
Member

nashif commented Jan 23, 2021

lowering priority as this happens only if /dev/null is writable or as root, which is not recommended.

@nashif nashif added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Jan 23, 2021
domo141 pushed a commit to domo141/zephyr that referenced this issue Jan 25, 2021
The _save_old() to return early if <filename> is not file (or symlink
to a file).

This is simplest alternative to avoid attempt to rename /dev/null
(which could succeed).

This also keeps fifos (perhaps nonexistent potential usage but
this is nicer).

If <filename> were directory or socket, after shutil.copyfile(),
writing to the file (by caller, _write_config()), would fail.

Fixes zephyrproject-rtos#31362

Co-authored-by: Marti Bolivar <[email protected]>
Signed-off-by: Tomi Ollila <[email protected]>
nashif pushed a commit that referenced this issue Jan 28, 2021
The _save_old() to return early if <filename> is not file (or symlink
to a file).

This is simplest alternative to avoid attempt to rename /dev/null
(which could succeed).

This also keeps fifos (perhaps nonexistent potential usage but
this is nicer).

If <filename> were directory or socket, after shutil.copyfile(),
writing to the file (by caller, _write_config()), would fail.

Fixes #31362

Co-authored-by: Marti Bolivar <[email protected]>
Signed-off-by: Tomi Ollila <[email protected]>
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: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants