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

mountinfo: fix path unescaping #16

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Jun 23, 2020

Moby PR moby/moby#38977 and containerd PR containerd/containerd#3159
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 containerd/containerd#4257

The solution is to stop using strconv.Unquote and write our own
specialized function to deal with escape sequences.

Unit tests added.

In addition, unescape the fstype and source fields (the first two after the - separator, since kernel escapes those as well.

@kolyshkin
Copy link
Collaborator Author

@thaJeztah PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah
Copy link
Member

@cpuguy83 PTAL

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Nit about function doc.
Might be nice to publish this for other uses as well (just a thought).

mountinfo/mountinfo_linux.go Outdated Show resolved Hide resolved
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>
Just noticed it while reading the kernel source code (function
show_mountinfo() in fs/proc_namespace.c; see also: show_type(),
mangle()) that the first two fields after the `-` separator are also
escaped.

While I haven't seen it in real life, it's better we unescape them.

No field-specific tests were added, and unescape() is tested by its own
test case.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Collaborator Author

Might be nice to publish this for other uses as well (just a thought).

Maybe, but I don't want it to be part of mountinfo. We can make something like moby/sys/filepath or moby/sys/linux or something like that later.

@kolyshkin
Copy link
Collaborator Author

Added a second commit, unescaping the fstype and source fields.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 2dee25b into moby:master Jun 24, 2020
@zhsj
Copy link
Contributor

zhsj commented Jun 25, 2020

I'm not sure what happens. The CI seems happy. But I fail to run test locally now.

$ git bisect run bash -c 'cd mountinfo; go test -run TestParseMountinfoWithSpaces'
running bash -c cd mountinfo; go test -run TestParseMountinfoWithSpaces
PASS
ok      github.com/moby/sys/mountinfo   0.002s
Bisecting: 1 revision left to test after this (roughly 1 step)
[2dee25b83acf7cab475f26a3a1da8b2a9a03e833] Merge pull request #16 from kolyshkin/fix-unescape
running bash -c cd mountinfo; go test -run TestParseMountinfoWithSpaces
--- FAIL: TestParseMountinfoWithSpaces (0.00s)
    mountinfo_linux_test.go:537: expected mountinfo.Info{ID:31, Parent:21, Major:0, Minor:23, Root:"/", Mountpoint:"/DATA/foo_bla_bla", Opts:"rw,relatime", Optional:"", Fstype:"cifs", Source:"//foo/BLA\\040BLA\\040BLA/", VfsOpts:"rw,sec=ntlm,cache=loose,unc=\\\\foo\\BLA"}, got &mountinfo.Info{ID:31, Parent:21, Major:0, Minor:23, Root:"/", Mountpoint:"/DATA/foo_bla_bla", Opts:"rw,relatime", Optional:"", Fstype:"cifs", Source:"//foo/BLA BLA BLA/", VfsOpts:"rw,sec=ntlm,cache=loose,unc=\\\\foo\\BLA"}
FAIL
exit status 1
FAIL    github.com/moby/sys/mountinfo   0.002s
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[0b171c288ece8a72ad48ee827ecaa4e53091e8d5] mountinfo: also unescape fstype and source
running bash -c cd mountinfo; go test -run TestParseMountinfoWithSpaces
--- FAIL: TestParseMountinfoWithSpaces (0.00s)
    mountinfo_linux_test.go:537: expected mountinfo.Info{ID:31, Parent:21, Major:0, Minor:23, Root:"/", Mountpoint:"/DATA/foo_bla_bla", Opts:"rw,relatime", Optional:"", Fstype:"cifs", Source:"//foo/BLA\\040BLA\\040BLA/", VfsOpts:"rw,sec=ntlm,cache=loose,unc=\\\\foo\\BLA"}, got &mountinfo.Info{ID:31, Parent:21, Major:0, Minor:23, Root:"/", Mountpoint:"/DATA/foo_bla_bla", Opts:"rw,relatime", Optional:"", Fstype:"cifs", Source:"//foo/BLA BLA BLA/", VfsOpts:"rw,sec=ntlm,cache=loose,unc=\\\\foo\\BLA"}
FAIL
exit status 1
FAIL    github.com/moby/sys/mountinfo   0.002s
0b171c288ece8a72ad48ee827ecaa4e53091e8d5 is the first bad commit
commit 0b171c288ece8a72ad48ee827ecaa4e53091e8d5
Author: Kir Kolyshkin <kolyshkin@gmail.com>
Date:   Tue Jun 23 14:12:13 2020 -0700

    mountinfo: also unescape fstype and source
    
    Just noticed it while reading the kernel source code (function
    show_mountinfo() in fs/proc_namespace.c; see also: show_type(),
    mangle()) that the first two fields after the `-` separator are also
    escaped.
    
    While I haven't seen it in real life, it's better we unescape them.
    
    No field-specific tests were added, and unescape() is tested by its own
    test case.
    
    Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

 mountinfo/mountinfo_linux.go | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
bisect run success

@zhsj
Copy link
Contributor

zhsj commented Jun 25, 2020

The CI is false positive...

image

@@ -511,6 +512,21 @@ func TestParseMountinfoWithSpaces(t *testing.T) {
Source: `//foo/BLA\040BLA\040BLA/`,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is the test that's failing

@thaJeztah
Copy link
Member

thaJeztah commented Jun 25, 2020

Wondering why CI didn't fail; could it be because we're using a sub shell?

sys/Makefile

Line 10 in 4a24c04

(cd $$p && go test -v .); \

Hm.. no idea; trying to reproduce with a very basic Makefile;

PACKAGES ?= mountinfo mount

.PHONY: test
test:
        for p in $(PACKAGES); do \
                (echo $$p && exit 1); \
        done
make test
for p in mountinfo; do \
		(echo $p && exit 1); \
	done
mountinfo
make: *** [test] Error 1

@zhsj
Copy link
Contributor

zhsj commented Jun 25, 2020

@thaJeztah
Copy link
Member

@zhsj thanks! just saw it after I posted my last comment

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

Successfully merging this pull request may close these issues.

4 participants