Fix parseInfoFile does not handle spaces in filenames #3159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ping @kolyshkin PTAL |
thaJeztah
commented
Apr 2, 2019
| Optional: "", | ||
| FSType: "cifs", | ||
| Source: `//foo/BLA\040BLA\040BLA/`, | ||
| VFSOptions: `rw,sec=ntlm,cache=loose,unc=\\foo\BLA`, |
There was a problem hiding this comment.
Note that this is truncating all options after the space; this is documented in parseInfoFile as expected, but I'm thinking of doing a follow-up to preserve the information that was removed (this might need some discussion, so I'm keeping that separate)
fuweid
reviewed
Apr 2, 2019
mount/mountinfo_linux.go
Outdated
| @@ -21,6 +21,7 @@ package mount | |||
| import ( | |||
| "bufio" | |||
| "fmt" | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
could we move it to next package group?
There was a problem hiding this comment.
oh! yes, looks like my editor inserted it
mount/mountinfo_linux_test.go
Outdated
| } | ||
|
|
||
| if len(infos) != len(expected) { | ||
| t.Fatalf("Expected %d entries, got %d", len(expected), len(infos)) |
There was a problem hiding this comment.
s/Expected/expected use lowercase here
|
I think we should use one commit here :) |
`/proc/self/mountinfo` uses `\040` for spaces, however, `parseInfoFile()`
did not decode those spaces in paths, therefore attempting to use `\040`
as a literal part of the path.
This patch un-quotes the `root` and `mount point` fields to fix
situations where paths contain spaces.
Note that the `mount source` field is not modified, given that
this field is documented (man `PROC(5)`) as:
filesystem-specific information or "none"
Which I interpreted as "the format in this field is undefined".
Reported-by: Daniil Yaroslavtsev <daniilyar@users.noreply.github.com>
Reported-by: Nathan Ringo <remexre@gmail.com>
Based-on-patch-by: Diego Becciolini <itizir@users.noreply.github.com>
Based-on-patch-by: Sergei Utinski <sergei-utinski@users.noreply.github.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_parseinfofile_parsing
branch
from
d6678cc
to
b1ea302
Compare
|
updated 👍
I kept the pkg/errors change separate as it was not directly related to the fix (and in case cherry-picking to release-branches would be more complicated because of those), but happy to squash if the maintainers prefer |
|
flaky case show up again 🤷♂️ @thaJeztah could you mind to repush again? thanks |
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_parseinfofile_parsing
branch
from
b1ea302
to
adc4fa2
Compare
|
pushed again 👍 |
Codecov Report
@@ Coverage Diff @@
## master #3159 +/- ##
==========================================
- Coverage 45.46% 42.38% -3.09%
==========================================
Files 104 115 +11
Lines 10198 12751 +2553
==========================================
+ Hits 4637 5404 +767
- Misses 4738 6513 +1775
- Partials 823 834 +11
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #3159 +/- ##
==========================================
- Coverage 45.46% 42.38% -3.09%
==========================================
Files 104 115 +11
Lines 10198 12751 +2553
==========================================
+ Hits 4637 5404 +767
- Misses 4738 6513 +1775
- Partials 823 834 +11
Continue to review full report at Codecov.
|
kolyshkin
added a commit
to kolyshkin/sys
that referenced
this pull request
Jun 23, 2020
Moby PR #38977 [1] and containerd PR #3159 [2] added the handing of escape sequences in mountinfo paths (root and mountpoint fields). Unfortunately, it also broke the handling of paths containing double quotes, as it was pointed out in [3]. The solution is to stop using strconv.Unquote and write our own specialized function to deal with escape sequences. Unit tests added. [1] moby/moby#38977 [2] containerd/containerd#3159 [3] containerd/containerd#4257 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added a commit
to kolyshkin/sys
that referenced
this pull request
Jun 23, 2020
Moby PR #38977 [1] and containerd PR #3159 [2] added the handing of escape sequences in mountinfo paths (root and mountpoint fields). Unfortunately, it also broke the handling of paths containing double quotes, as it was pointed out in [3]. The solution is to stop using strconv.Unquote and write our own specialized function to deal with escape sequences. Unit tests added. [1] moby/moby#38977 [2] containerd/containerd#3159 [3] containerd/containerd#4257 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added a commit
to kolyshkin/sys
that referenced
this pull request
Jun 23, 2020
Moby PR #38977 [1] and containerd PR #3159 [2] added the handing of escape sequences in mountinfo paths (root and mountpoint fields). Unfortunately, it also broke the handling of paths containing double quotes, as it was pointed out in [3]. The solution is to stop using strconv.Unquote and write our own specialized function to deal with escape sequences. Unit tests added. [1] moby/moby#38977 [2] containerd/containerd#3159 [3] containerd/containerd#4257 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
equivalent to moby/moby#38977, for containerd
relates to moby/moby#38458
relates to kubernetes/kubernetes#35062
/proc/self/mountinfouses\040for spaces, however,parseInfoFile()did not decode those spaces in paths, therefore attempting to use
\040as a literal part of the path.
This patch un-quotes the
rootandmount pointfields to fixsituations where paths contain spaces.
Note that the
mount sourcefield is not modified, given thatthis field is documented (man
PROC(5)) as:Which I interpreted as "the format in this field is undefined".
Thanks to @remexre for reporting, and @itizir, @sergei-utinski for suggesting the patch.