Skip to content

extmod/vfs_posix: Fix relative paths on non-root VFS #12137

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

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

cwalther
Copy link
Contributor

VfsPosix has a couple of bugs regarding handling of relative paths and chdir()/getcwd() that surface when it is instanced with a non-empty root path (i.e. only a subtree of the outer filesystem is exposed to Python).

They are somewhat entangled, but I tried my best to separate the fixes into individually reviewable commits, each with first a failing test that demonstrates what is going wrong, followed by fixes that makes it pass. (The tricky part of that was to get the fixes in the right order and craft the tests such that they actually pass after the respective fix, rather than continuing to fail due to the remaining bugs.)

  1. A VfsPosix created with a relative root path gets confused when chdir() is called on it and becomes unable to properly resolve absolute paths, because changing directories effectively shifts its root. The simplest fix for that would be to say “don’t do that”, but since the unit tests themselves do it, fix it by making a relative path absolute before storing it.

  2. The unwritten API contract expected of a VFS by mp_vfs_lookup_path() is that paths passed in are relative to the root of the VFS if they start with / and relative to the current directory of the VFS otherwise. This is not correctly implemented in VfsPosix for instances with a non-empty root – all paths are interpreted relative to the root. Fix that. Since VfsPosix tracks its CWD using the “external” CWD of the Unix process, the correct handling for relative paths is to pass them through unmodified.

  3. The unwritten API contract expected of a VFS.getcwd() by mp_vfs_getcwd() is that its return value should be either "" or "/" when the CWD is at the root of the VFS and otherwise start with a slash and not end with a slash. This is not correctly implemented in VfsPosix for instances with a non-empty root – the required leading slash, if any, is cut off because the root length includes a trailing slash. This results in missing slashes in the middle of the return value of os.getcwd() or in uninitialized garbage from beyond a string’s null terminator when the CWD is at the VFS root.

The matter is complicated by the fact that there are some faulty existing tests that only pass by accident due to bug 2. Unmodified, they would fail after the fix for bug 2 (5th commit), however that is insignificant because they actually test an unrealistic situation. Fixing them in the 3rd commit to match the realistic situation makes them pass with bug 2 fixed (and also without, when done in this order, because after the 2nd commit, bug 2 has no effect in that case).

The unrealistic situation is that VfsPosix methods are called with relative paths while the current working directory is somewhere outside of the root of the VFS. In the intended use of VFS objects via os.mount() (as opposed to calling methods directly as the tests do), this never happens, as mp_vfs_lookup_path() directs incoming calls to the VFS that contains the CWD. Fix this by explicitly changing the working directory to the root of the VFS before calling methods on it, as the subsequent relative path accesses expect.

@github-actions
Copy link

github-actions bot commented Jul 31, 2023

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +168 +0.021% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@cwalther
Copy link
Contributor Author

I’ll take a look at the Windows failures tomorrow (I didn’t expect having to care about Windows in something named “posix”). The coverage failure seems out of my hands.

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #12137 (7be16e0) into master (86c7b95) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #12137   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         158      158           
  Lines       20954    20965   +11     
=======================================
+ Hits        20617    20629   +12     
+ Misses        337      336    -1     
Files Coverage Δ
extmod/vfs_posix.c 94.41% <100.00%> (+0.92%) ⬆️

... and 1 file with indirect coverage changes

@cwalther
Copy link
Contributor Author

cwalther commented Aug 1, 2023

Windows fixes are in, this is good to go as far as I am concerned.

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Aug 10, 2023
@cwalther
Copy link
Contributor Author

Addressed review comments and rebased on master (which required no changes).

@@ -130,6 +130,70 @@ def write_files_without_closing():
except OSError:
print("remove OSError")

# construct new VfsPosix with absolute path argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit "Add more tests for relative paths, currently failing." adds a lot to the existing test file and makes it too complex. So please can you put these new tests in a separate test file, maybe vfs_posix_paths.py.

Also, we usually have tests in the same commit as the commit that fixes the issue. So please also try to do that here if it makes sense (I understand you made changes to tests that were accidentally passing, and those can indeed stay in a separate commit.).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do. Probably not before the weekend.

(In case you were wondering about the separate commits, there is a reason I did that. I always do it that way, because it makes it easier to check that 1. something is wrong (by checking out the first commit with the failing test) and 2. the change actually fixes it (by checking out the second commit with the fix). But of course I willl conform to the project style.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fully on board with making lots of small commits and separating out functionality into different commits. In general we try to do that in this repo.

But for this particular case, I felt that it was a bit confusing which test went with which fix. And I think it would be easier for someone reading the git history if the fix and test were in the same commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Apologies it took me several weekends.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all.

A VfsPosix created with a relative root path would get confused when
chdir() was called on it and become unable to properly resolve absolute
paths, because changing directories effectively shifted its root. The
simplest fix for that would be to say "don't do that", but since the
unit tests themselves do it, fix it by making a relative path absolute
before storing it.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
These tests test an unrealistic situation and only pass by accident due
to a bug. The upcoming fix for the bug would make them fail.

The unrealistic situation is that VfsPosix methods are called with
relative paths while the current working directory is somewhere outside
of the root of the VFS. In the intended use of VFS objects via
os.mount() (as opposed to calling methods directly as the tests do),
this never happens, as mp_vfs_lookup_path() directs incoming calls to
the VFS that contains the CWD.

Make the testing situation realistic by changing the working directory
to the root of the VFS before calling methods on it, as the subsequent
relative path accesses expect.

Thanks to the preceding commit, the tests still pass, but still for the
wrong reason. The following commit "Fix relative paths on non-root VFS"
will make them pass for the correct reason.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
The unwritten API contract expected of a VFS by mp_vfs_lookup_path() is
that paths passed in are relative to the root of the VFS if they start
with '/' and relative to the current directory of the VFS otherwise.
This was not correctly implemented in VfsPosix for instances with a
non-empty root - all paths were interpreted relative to the root. Fix
that. Since VfsPosix tracks its CWD using the "external" CWD of the Unix
process, the correct handling for relative paths is to pass them through
unmodified.

Also, when concatenating absolute paths, fix an off-by-one resulting in
a harmless double slash (the root path already has a trailing slash).

Signed-off-by: Christian Walther <cwalther@gmx.ch>
The unwritten API contract expected of a VFS.getcwd() by mp_vfs_getcwd()
is that its return value should be either "" or "/" when the CWD is at
the root of the VFS and otherwise start with a slash and not end with a
slash. This was not correctly implemented in VfsPosix for instances with
a non-empty root - the required leading slash, if any, was cut off
because the root length includes a trailing slash. This would result in
missing slashes in the middle of the return value of os.getcwd() or in
uninitialized garbage from beyond a string's null terminator when the
CWD was at the VFS root.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
Signed-off-by: Christian Walther <cwalther@gmx.ch>
@cwalther
Copy link
Contributor Author

  • Split added tests out into a new file vfs_posix_paths.py.
  • Squashed test and corresponding fix into the same commit.
  • Rebased on master (which required no changes).

@dpgeorge dpgeorge merged commit 7be16e0 into micropython:master Oct 20, 2023
@dpgeorge
Copy link
Member

Thanks for updating and making clean and independent commits. Merged as-is.

@cwalther
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants