snapd mountinfo parser doesn't handle the special "\040(deleted)" suffix

[Zygmunt Krynicki]

The kernel can include a special in one of the strings returned in /proc/pid/mountinfo. This suffix is used when one of the objects referring to the mount has been deleted and the mount is no longer functional.

I realised this by reviewing a patch https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=86673b3a46

We don't currently expect to see such mount points during normal operation but then again, we should handle it explicitly.

[Arseniy Aharonov (arsenique)]

I've succeeded to reproduce the behaviour described by this ticket. One shall perform the following steps to reproduce it:
```
# unshare -m
# echo foo > foo; touch bar baz quux
# mount -B foo bar
# mount -B bar baz
# grep foo /proc/self/mountinfo
56 38 8:7 /tmp/foo /tmp/bar ...
57 38 8:7 /tmp/foo /tmp/baz ...

# rm foo
# grep foo /proc/self/mountinfo
56 38 8:7 /tmp/foo//deleted /tmp/bar ...
57 38 8:7 /tmp/foo//deleted /tmp/baz ...
```

[Arseniy Aharonov (arsenique)]

The related functionality in c-sources of snapd actually duplicates what does the ''mnt_table_parse_fstab()'' API that can be easily populated with ''<libmount/libmount.h>''. This will fix all possible problem that might be related to the reported issue.

[Zygmunt Krynicki (zyga)]

Yes but at the time we didn't choose to use libmount as that would introduce another dependency that would have to be up-to-date in the target distribution.

[Arseniy Aharonov (arsenique)]

Could you explain in more details please? What are we trying to avoid by duplicating the code? Which dependency are we trying to avoid? Aren't we talking about the dependency that is already resolved?
This packages is mandatory for the Kernel to be functional. It is always present.

[Zygmunt Krynicki (zyga)]

It's not about making sure the dependency is present. At the time this code was written, there were two concerns:

- the version of libmount across distributions was not uniform
- using libmount directly required additional apparmor profile permissions

This decision can be re-visited by the snapd team, I'm just providing the historic context.

[Arseniy Aharonov (arsenique)]

Well, till now three potential regions might be affected by this potential issue:
1. C-logic related to parsing of Mountinfo
2. bash scripts that are used to cleanup after a snap is removed
3. GoLang logic related to parsing of Mountinfo

[Arseniy Aharonov (arsenique)]

For the first phase, it was decided to leave the C-logic without changes due to complicated state of dependencies of this code. The code in question is very sensitive to security issues and demands very heavy auditing to prevent the risk of security breaches.

Bash scripts and Go-Lang require preliminary refactoring prior to fixes, to prevent maintainability issues (e.g. code duplication).

[Arseniy Aharonov (arsenique)]

Hi,

After analyzing the `unitl-linux` of Ubuntu LTS, I've seen that the aforementioned patch with the fix is included.
However, the attempt to reproduce the failure shows that the `/(deleted)` prefix is attached to the root filed of the mount-info entry. Which made me think that what we need to focus on is to check how snapd itself is affected when parsing mountinfo records that reflect the status of broken link.

I've reproduced the failure once again on a VM and used snapd osutil package to load the "corrupted" moountinfo data, the same way as snapd does. Then, I printed the contents of the []*MountInfoEntry in JSON format (please see the attached reproduced_MountInfoEntry_structure.json). As one can see, snapd succeeded to process such file without crashing, and similar to reproduced results in command-line, the output is affected the way that the `/(deleted)` prefix is attached to the root filed. This means that we are not at risk of crashing due to this phenomena in the file.
Taking into account that this data doesn't crash snapd, it worth checking how this field is used by snapd, and currently there is no occurrence within the snapd code where this field is used rather then simply compared to '/'.

All this makes me to conclude that this doesn't pose any risks to at least go-part of snapd.

Kind regards,
Arseniy