osutil: fix parsing super-block options with spaces #12899
Conversation
On any Windows + WSL 2 + Docker system, running any snap command is immediately preceded by the two errors: 2023/06/17 23:59:52.071511 system_key.go:129: cannot determine nfs usage in generateSystemKey: cannot parse mountinfo: incorrect number of tail fields, expected 3 but found 4 2023/06/17 23:59:52.080033 cmd_run.go:1046: WARNING: cannot create user data directory: cannot determine SELinux status: failed to obtain SELinux mount path: incorrect number of tail fields, expected 3 but found 4 Those errors confuse tools that parse program output and generally cause a lot of havoc. The root of the issue is caused by this specific entry in /proc/self/mountinfo (not wrapped to preserve the problematic part more prominently): 1146 77 0:149 / /Docker/host rw,noatime - 9p drvfs rw,dirsync,aname=drvfs;path=C:\Program Files\Docker\Docker\resources;symlinkroot=/mnt/,mmap,access=client,msize=262144,trans=virtio Note that the super-block mount option contains a key=value pair where the value contains un-escaped spaces. Historically mountinfo has been an utter mess to parse, with several bugs in both the userspace parsing and several bugs in the fragile kernel interface generating the contents of said file. To work around this problem allow spaces in the super-block options and silently parse them as if they had been escaped correctly. Signed-off-by: Zygmunt Krynicki <[email protected]>
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #12899 +/- ##
=======================================
Coverage 78.59% 78.60%
=======================================
Files 992 992
Lines 123110 123111 +1
=======================================
+ Hits 96760 96767 +7
+ Misses 20247 20244 -3
+ Partials 6103 6100 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
On any Windows + WSL 2 + Docker system, running any snap command is immediately preceded by the two errors: 2023/06/17 23:59:52.071511 system_key.go:129: cannot determine nfs usage in generateSystemKey: cannot parse mountinfo: incorrect number of tail fields, expected 3 but found 4 2023/06/17 23:59:52.080033 cmd_run.go:1046: WARNING: cannot create user data directory: cannot determine SELinux status: failed to obtain SELinux mount path: incorrect number of tail fields, expected 3 but found 4 Those errors confuse tools that parse program output and generally cause a lot of havoc. The root of the issue is caused by this specific entry in /proc/self/mountinfo (not wrapped to preserve the problematic part more prominently): 1146 77 0:149 / /Docker/host rw,noatime - 9p drvfs rw,dirsync,aname=drvfs;path=C:\Program Files\Docker\Docker\resources;symlinkroot=/mnt/,mmap,access=client,msize=262144,trans=virtio Note that the super-block mount option contains a key=value pair where the value contains un-escaped spaces. Historically mountinfo has been an utter mess to parse, with several bugs in both the userspace parsing and several bugs in the fragile kernel interface generating the contents of said file. To work around this problem allow spaces in the super-block options and silently parse them as if they had been escaped correctly. Signed-off-by: Zygmunt Krynicki <[email protected]>
mvo5
added
the
Needs security review
|
Hey @alexmurray - this is a small patch that makes snaps usable on WSL if you also happen to have Docker installed - please tell me if the way the change is made is sensible, or of some other approach is viable in Your eyes. Thanks! |
|
hey @zyga - apologies I'm taking to long to get to this - I hope to take a proper look tomorrow so with any luck it should be done on my side in 24h |
There was a problem hiding this comment.
LGTM - whilst this change adds some complexity and makes this already fragile code a bit more fragile, I think it is definitely worth it to support this reasonably common use-case. The nature of /proc/mountinfo lacking good uniform escaping etc means we have to try and be more lenient. Also this is already a trusted input so it doesn't add any extra risk to add this complexity.
Thanks @zyga.
alexmurray
removed
the
Needs security review
|
@mvo5 could this be considered for the next release? |
* rename "last"->"allow_spaces_in_field" in parse_next_string_field_ex()
* add comments to `parse_{next,last}_string_field()`
On any Windows + WSL 2 + Docker system, running any snap command is immediately preceded by the two errors:
Those errors confuse tools that parse program output and generally cause a lot of havoc.
The root of the issue is caused by this specific entry in /proc/self/mountinfo (not wrapped to preserve the problematic part more prominently):
Note that the super-block mount option contains a key=value pair where the value contains un-escaped spaces. Historically mountinfo has been an utter mess to parse, with several bugs in both the userspace parsing and several bugs in the fragile kernel interface generating the contents of said file.
To work around this problem allow spaces in the super-block options and silently parse them as if they had been escaped correctly.