Cope with double quotes in Linux Mountinfo #4325

johannesfrey · 2020-06-16T07:05:01Z

This PR resolves the double quote problem described in issue #4257

theopenlab-ci · 2020-06-16T07:09:16Z

Build succeeded.

containerd-build-arm64 : FAILURE in 1m 36s (non-voting)

AkihiroSuda · 2020-06-16T07:20:33Z

mount/mountinfo_linux_test.go

@@ -439,6 +439,9 @@ const (

 	mountInfoWithSpaces = `486 28 252:1 / /mnt/foo\040bar rw,relatime shared:243 - ext4 /dev/vda1 rw,data=ordered
 31 21 0:23 / /DATA/foo_bla_bla rw,relatime - cifs //foo/BLA\040BLA\040BLA/ rw,sec=ntlm,cache=loose,unc=\\foo\BLA BLA BLA,username=my_login,domain=mydomain.com,uid=12345678,forceuid,gid=12345678,forcegid,addr=10.1.30.10,file_mode=0755,dir_mode=0755,nounix,rsize=61440,wsize=65536,actimeo=1`
+
+	mountInfoWithDoubleQuotes = `1046 30 253:1 /tmp/bar /var/lib/kubelet/pods/98d150a4-d814-4d52-9068-b10f62d7a895/volumes/kubernetes.io~empty-dir/tmp-dir/"var rw,relatime shared:1 - ext4 /dev/mapper/ubuntu--vg-root rw,errors=remount-ro


Please also test with a path that contains \" (backslash + quote)?

Mounting a directory containing a backslash will display it ascii escaped in /proc/self/mountinfo:

mkdir '/tmp/\"bar\"' mkdir /tmp/bar sudo mount --bind /tmp/bar '/tmp/\"bar\"' cat /proc/self/mountinfo | grep bar 1100 30 253:1 /tmp/bar /tmp/\134"bar\134" rw,relatime shared:1 - ext4 /dev/mapper/ubuntu--vg-root rw,errors=remount-ro

So I added a testcase that receives the backslash ascii character. Did you mean it this way? It seems that a literal \" cannot show up in the output of /proc/self/mountinfo.

Yes, thanks

Just to double-check; the field (as a whole) is never quoted? ("/var/lib/....."), so no exception is needed for leading/trailing quote?

oh (sorry for the noise); would single quotes have the same problem?

No problem, valid questions 😉. For the latter I added some more test cases.

It seems that a literal " cannot show up in the output of /proc/self/mountinfo

It's not really clear to me then why it is using unquote at all

It's not really clear to me then why it is using unquote at all

So, mountinfo entries quote the following characters, using escape sequences with their octal ascii codes:

(i.e. space) -- as \040

\t (i.e. tab) -- as \011

\n (i.e. newline) -- as \012

\\ (i.e. backslash) -- as \134

The unquote was added (by @thaJeztah AFAIR) to convert these escape sequences back to spaces, tabs etc. It does that but messes up double quotes.

I think the solution is to manually interpret the above sequences.

I think the solution is to manually interpret the above sequences.

I mean, write our own function to de-escape \040, \011, \012 and \134 instead of using strconv.Unquite

yeah, I agree with that. Looking at the implementation of strconv.Unquote makes it seem like it is unnecessary to quote then use that. If these are the case we need to test against, then we should add them to the tests.

If you look at the strconv.Unquote function you will see it first just does some checks whether it contains some of these special characters. This could probably do the same thing and if there is no backslash, just return.

theopenlab-ci · 2020-06-16T09:59:33Z

Build succeeded.

containerd-build-arm64 : FAILURE in 1m 36s (non-voting)

theopenlab-ci · 2020-06-16T10:06:18Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 06s (non-voting)

thaJeztah · 2020-06-16T10:08:53Z

/cc @kolyshkin ptal

Signed-off-by: Johannes Frey <me@johannes-frey.de>

theopenlab-ci · 2020-06-16T11:11:52Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 06s (non-voting)

estesp

LGTM

dmcgowan · 2020-06-17T22:35:15Z

mount/mountinfo_linux.go

 		if err != nil {
 			return nil, errors.Wrapf(err, "parsing '%s' failed: unable to unquote root field", fields[3])
 		}
-		p.Mountpoint, err = strconv.Unquote(`"` + fields[4] + `"`)
+		p.Mountpoint, err = strconv.Unquote(`"` + strings.Replace(fields[4], `"`, `\"`, -1) + `"`)


Would switching to backquote solve this out the extra replace. https://golang.org/pkg/strconv/#Unquote

`"` -> "`"

Unless of course backquote also needs to be supported...

It seems so:

› cat /proc/self/mountinfo | grep "foo\|bar" 1047 30 253:1 /tmp/`foo` /tmp/bar rw,relatime shared:1 - ext4 /dev/mapper/ubuntu--vg-root rw,errors=remount-ro

I added a test case containing backticks as well.

with backticks, \040 is not expanded and this brings back the bug that the initial unquote solves.

theopenlab-ci · 2020-06-18T06:36:34Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 31s (non-voting)

Signed-off-by: Johannes Frey <me@johannes-frey.de>

theopenlab-ci · 2020-06-18T06:40:57Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 21s (non-voting)

kolyshkin · 2020-06-18T22:13:05Z

Left a suggestion (#4325 (comment)) on how to fix this effectively.

WRT test cases -- I would separate out the filename unquote function and write test cases for it.

In general, I would love this repo to switch to using github.com/moby/sys/mountinfo

kolyshkin · 2020-06-23T00:45:08Z

Fixed the same bug in github.com/moby/sys/mountinfo, see moby/sys#16

It would be beneficial to reuse that repo here; if not, the fix can be ported.

dmcgowan · 2020-06-23T00:55:06Z

Doing a direct escape seems like the right thing to do here. Adding a dependency doesn't make much sense for this change, especially since we have avoided bringing in this dependency in the past.

@kolyshkin is there any documentation you can link to related to that set of characters or just the use of octal encoding in that file?

kolyshkin · 2020-06-23T01:36:07Z

Adding a dependency doesn't make much sense for this change

I am not proposing the use of moby/sys/mountinfo for this particular change, but rather for overall simplification and easement of maintenance burden. containerd's mountinfo is a fork of the one in moby, and now every bugfix, feature, or improvement needs to be done in two places.

since we have avoided bringing in this dependency in the past

You're probably talking about moby/moby. Using packages from moby/moby is not possible, as it will lead to circular dependencies. This was one of the main reasons why moby/sys was born -- it does not have any outside deps and can be used by any project. After cross-porting some stuff from moby and containerd and back, I think the best solution long-term is to have a single place for such stuff.

I remember @crosbymichael at least considered the idea of using moby/sys in containerd earlier.

is there any documentation you can link to related to that set of characters or just the use of octal encoding in that file?

Probably not (there's proc(5) man page that describes mountinfo, but it does not mention escaping).

The source for what I wrote earlier is kernel source code, specifically, show_mountinfo in fs/proc_namespace.c, which calls seq_dentry in fs/seq_file.c (with the last argument being the characters to escape), which calls mangle_path in the same file, which does the actual escaping.

https://github.com/torvalds/linux/blob/dd0d718152e4c65b173070d48ea9dfc06894c3e5/fs/proc_namespace.c#L146

https://github.com/torvalds/linux/blob/dd0d718152e4c65b173070d48ea9dfc06894c3e5/fs/seq_file.c#L399-L428

kolyshkin · 2020-06-23T21:21:39Z

is there any documentation you can link to related to that set of characters or just the use of octal encoding in that file?

In addition to source code references above, today I found https://www.kernel.org/doc/Documentation/filesystems/seq_file.txt (look for seq_escape) which might add some more context.

dmcgowan

LGTM

This is a small PR with mostly tests, let's get this merged then moving away from Unquote can be a follow up

crosbymichael

LGTM

This adds tests for handling paths containing double quotes, and is the equivalent to the changes made in containerd/containerd#4325 to fix the problem described in containerd/containerd#4257 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This adds tests for handling paths containing double quotes, and is the equivalent of the tests added in containerd/containerd#4325 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

AkihiroSuda reviewed Jun 16, 2020

View reviewed changes

johannesfrey force-pushed the mountinfo-linux-double-quotes branch from 8f23ce2 to 33d6abc Compare June 16, 2020 10:03

johannesfrey added 3 commits June 16, 2020 13:06

Cope with double quotes in Linux Mountinfo

87f9fdb

Signed-off-by: Johannes Frey <me@johannes-frey.de>

Add testcase containing mountpoint with escaped backslash

cb91b17

Signed-off-by: Johannes Frey <me@johannes-frey.de>

Add more test cases with single quotes

8897e15

Signed-off-by: Johannes Frey <me@johannes-frey.de>

johannesfrey force-pushed the mountinfo-linux-double-quotes branch from 33d6abc to 8897e15 Compare June 16, 2020 11:07

estesp approved these changes Jun 16, 2020

View reviewed changes

dmcgowan reviewed Jun 17, 2020

View reviewed changes

Add test case with backticks

ee734e8

Signed-off-by: Johannes Frey <me@johannes-frey.de>

johannesfrey force-pushed the mountinfo-linux-double-quotes branch from 5d09a5b to ee734e8 Compare June 18, 2020 06:37

dmcgowan approved these changes Jun 24, 2020

View reviewed changes

dmcgowan assigned AkihiroSuda Jun 24, 2020

crosbymichael approved these changes Jun 24, 2020

View reviewed changes

crosbymichael merged commit c751807 into containerd:master Jun 24, 2020

kzys mentioned this pull request Jul 16, 2020

Containerd cannot create containers that mount paths containing double quotes #4257

Closed

thaJeztah mentioned this pull request Sep 22, 2020

mountinfo: add test-cases for double quotes in Linux Mountinfo moby/sys#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cope with double quotes in Linux Mountinfo #4325

Cope with double quotes in Linux Mountinfo #4325

johannesfrey commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

AkihiroSuda Jun 16, 2020

johannesfrey Jun 16, 2020 •

edited

Loading

AkihiroSuda Jun 16, 2020

thaJeztah Jun 16, 2020

thaJeztah Jun 16, 2020

johannesfrey Jun 16, 2020

dmcgowan Jun 18, 2020

kolyshkin Jun 18, 2020 •

edited

Loading

kolyshkin Jun 18, 2020

dmcgowan Jun 19, 2020

theopenlab-ci bot commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

thaJeztah commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

estesp left a comment

dmcgowan Jun 17, 2020

johannesfrey Jun 18, 2020 •

edited

Loading

kolyshkin Jun 18, 2020

theopenlab-ci bot commented Jun 18, 2020

theopenlab-ci bot commented Jun 18, 2020

kolyshkin commented Jun 18, 2020 •

edited

Loading

kolyshkin commented Jun 23, 2020

dmcgowan commented Jun 23, 2020

kolyshkin commented Jun 23, 2020 •

edited

Loading

kolyshkin commented Jun 23, 2020

dmcgowan left a comment

crosbymichael left a comment

Cope with double quotes in Linux Mountinfo #4325

Cope with double quotes in Linux Mountinfo #4325

Conversation

johannesfrey commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

Choose a reason for hiding this comment

johannesfrey Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theopenlab-ci bot commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

thaJeztah commented Jun 16, 2020

theopenlab-ci bot commented Jun 16, 2020

estesp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannesfrey Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theopenlab-ci bot commented Jun 18, 2020

theopenlab-ci bot commented Jun 18, 2020

kolyshkin commented Jun 18, 2020 • edited Loading

kolyshkin commented Jun 23, 2020

dmcgowan commented Jun 23, 2020

kolyshkin commented Jun 23, 2020 • edited Loading

kolyshkin commented Jun 23, 2020

dmcgowan left a comment

Choose a reason for hiding this comment

crosbymichael left a comment

Choose a reason for hiding this comment

johannesfrey Jun 16, 2020 •

edited

Loading

kolyshkin Jun 18, 2020 •

edited

Loading

johannesfrey Jun 18, 2020 •

edited

Loading

kolyshkin commented Jun 18, 2020 •

edited

Loading

kolyshkin commented Jun 23, 2020 •

edited

Loading