cmd/libsnap: fix parsing of empty mountinfo fields

[zyga]

This patch fixes an apparently unexpected kernel behavior whereby
a tmpfs with empty "mount source" (which is customarily set to "none"
by various tools) results in a mountinfo field with two consecutive
spaces. That is, empty fields are just printed as-is.

The parser is adjusted and a new unit test is added to cover the case
where this was discovered.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[stolowski]

I'm unclear why is this needed again here? Can you explain?

[zyga]

The test checks if the first character of the field is a space. This is only true if the field is empty and so this gets special-cased as scanf style functions cannot parse empty strings with %s format character.

[stolowski]

Ok, sorry, I should be more clear: my question is whether this is needed here if same test is done on line 190?

[zyga]

Ah, sorry. I looked at the wrong part of the diff.

This test is to compensate the change that was done to sscanf call. Earlier we parsed %s %n and now we parse %s%n. This makes us scan each field correctly. For a visual look at how this behaves:

The # characters are poison bytes that were not written to by the parse routine.
The @ characters are NUL ('\0') bytes.

zyga@fyke:~/snapd/cmd$ make -j libsnap-confine-private/unit-tests && ./libsnap-confine-private/unit-tests -p /mountinfo/parse_mountinfo_entry/empty_source
gcc -DHAVE_CONFIG_H -I.    -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -D_ENABLE_FAULT_INJECTION -g -O2 -MT libsnap-confine-private/libsnap_confine_private_unit_tests-mountinfo-test.o -MD -MP -MF libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-mountinfo-test.Tpo -c -o libsnap-confine-private/libsnap_confine_private_unit_tests-mountinfo-test.o `test -f 'libsnap-confine-private/mountinfo-test.c' || echo './'`libsnap-confine-private/mountinfo-test.c
gcc -DHAVE_CONFIG_H -I.    -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -D_ENABLE_FAULT_INJECTION -g -O2 -MT libsnap-confine-private/libsnap_confine_private_unit_tests-utils-test.o -MD -MP -MF libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-utils-test.Tpo -c -o libsnap-confine-private/libsnap_confine_private_unit_tests-utils-test.o `test -f 'libsnap-confine-private/utils-test.c' || echo './'`libsnap-confine-private/utils-test.c
mv -f libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-utils-test.Tpo libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-utils-test.Po
mv -f libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-mountinfo-test.Tpo libsnap-confine-private/.deps/libsnap_confine_private_unit_tests-mountinfo-test.Po
gcc -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -D_ENABLE_FAULT_INJECTION -g -O2   -o libsnap-confine-private/unit-tests libsnap-confine-private/libsnap_confine_private_unit_tests-classic-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-cleanup-funcs-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-error-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-fault-injection-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-locking-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-mount-opt-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-mountinfo-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-privs-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-secure-getenv-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-snap-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-string-utils-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-test-utils.o libsnap-confine-private/libsnap_confine_private_unit_tests-test-utils-test.o libsnap-confine-private/libsnap_confine_private_unit_tests-unit-tests-main.o libsnap-confine-private/libsnap_confine_private_unit_tests-unit-tests.o libsnap-confine-private/libsnap_confine_private_unit_tests-utils-test.o -lglib-2.0  -Wl,-Bstatic -lcap -Wl,-Bdynamic
/mountinfo/parse_mountinfo_entry/empty_source: Input buffer (first), with offset arrow
Output buffer (second)
 ------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>################################################################################<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 --------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@#################################################################<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 --------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@#######################<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 --------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@###########<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 ----------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@-@#########<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 ----------------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@@@tmpfs@###<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 -----------------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@@@tmpfs@@##<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 -------------------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@@@tmpfs@@rw<
>================================================================================<
Input buffer (first), with offset arrow
Output buffer (second)
 -------------------------------------------------------------------------------v
>304 301 0:45 / /snap/test-snapd-content-advanced-plug/x1 rw,relatime - tmpfs  rw<
>#############/@/snap/test-snapd-content-advanced-plug/x1@rw,relatime@@@tmpfs@@rw<
>================================================================================<

[codecov-io]

Codecov Report

Merging #4109 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4109      +/-   ##
==========================================
- Coverage   75.95%   75.95%   -0.01%     
==========================================
  Files         440      440              
  Lines       38427    38427              
==========================================
- Hits        29189    29188       -1     
- Misses       7224     7225       +1     
  Partials     2014     2014

Impacted Files	Coverage Δ
interfaces/sorting.go	92.3% <0%> (-1.1%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db1fc27...98f5748. Read the comment docs.

[jdstrand]

The two spaces between 'tmpfs' and 'rw': is this guaranteed to denote an empty source? Ie, is it an implementation artifact or is it part of the specification for mountinfo? This seems scarily brittle at first glance.

[zyga]

It is just how the kernel is implemented. Reading the specification it is neither violating it nor explicitly explained that it behaves this way. IMO this is a kernel bug but the way countess kernel interfaces are defined it doesn’t stand out. I for one would love to see standardized serializer but this is unlikely to happen anytime soon.

I looked at how this is implemented and it is a %s with some escape logic for spaces and other characters.

[bboozzoo]

Slightly uneasy as we're not passing the size of line buffer as input to parse_next_string_field or at least verifying that there's still input to consume. In theory this should work, the kernel probably won't pass us some random garbage. Other than that, I didn't spot anything obviously wrong with the code.