[pbs-devel] [PATCH] fix #2882: correctly parse optional fields from mountinfo

[pbs-devel] [PATCH] fix #2882: correctly parse optional fields from mountinfo

[Stefan Reiter]

As per the linux kernel documentation[0], the "optional fields" (called
"tags" in our parser) are not comma-separated, but are a variable number
(0 or more) of space-separated fields, always followed by a dash to mark
the end.

Fix the /proc/<pid>/mountinfo parser to correctly parse these and add
test cases (l3 is the line that produced the original warning from the
bug report).

[0] https://www.kernel.org/doc/html/latest/filesystems/proc.html#proc-pid-mountinfo-information-about-mounts

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

I still think the way we parse here (with the fixed next()'s) is kind of weird,
but I don't have a better idea and the rest of the "mountinfo" format looks
pretty set in stone, so I suppose it's fine.

 proxmox/src/sys/linux/procfs/mountinfo.rs | 53 ++++++++++++++++++-----
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/proxmox/src/sys/linux/procfs/mountinfo.rs b/proxmox/src/sys/linux/procfs/mountinfo.rs
index 55f74c2..b57f132 100644
--- a/proxmox/src/sys/linux/procfs/mountinfo.rs
+++ b/proxmox/src/sys/linux/procfs/mountinfo.rs
@@ -135,17 +135,18 @@ impl Entry {
             root: OsStr::from_bytes(next()?).to_owned().into(),
             mount_point: OsStr::from_bytes(next()?).to_owned().into(),
             mount_options: OsStr::from_bytes(next()?).to_owned(),
-            tags: next()?.split(|b| *b == b',').try_fold(
-                Vec::new(),
-                |mut acc, tag| -> Result<_, Error> {
-                    acc.push(Tag::parse(tag)?);
-                    Ok(acc)
-                },
-            )?,
-            fs_type: std::str::from_utf8({
-                next()?;
-                next()?
-            })?
+            tags: {
+                let mut tags = Vec::new();
+                loop {
+                    let tval = next()?;
+                    if tval == b"-" {
+                        break;
+                    }
+                    tags.push(Tag::parse(tval)?);
+                }
+                tags
+            },
+            fs_type: std::str::from_utf8(next()?)?
             .to_string(),
             mount_source: next().map(|src| match src {
                 b"none" => None,
@@ -350,6 +351,36 @@ fn test_entry() {
         "rw,fd=26,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=27726"
     );
 
+    // test different tag configurations
+    let l3: &[u8] =
+        b"225 224 0:46 / /proc rw,nosuid,nodev,noexec,relatime - proc proc rw";
+    let entry = Entry::parse(l3).expect("failed to parse third mountinfo test entry");
+    assert_eq!(entry.tags, &[]);
+
+    let l4: &[u8] =
+        b"48 32 0:43 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime \
+          shared:5 master:7 propagate_from:2 unbindable \
+          - cgroup cgroup rw,blkio";
+    let entry = Entry::parse(l4).expect("failed to parse fourth mountinfo test entry");
+    assert_eq!(entry.tags, &[
+        Tag {
+            tag: OsString::from("shared"),
+            value: Some(OsString::from("5")),
+        },
+        Tag {
+            tag: OsString::from("master"),
+            value: Some(OsString::from("7")),
+        },
+        Tag {
+            tag: OsString::from("propagate_from"),
+            value: Some(OsString::from("2")),
+        },
+        Tag {
+            tag: OsString::from("unbindable"),
+            value: None,
+        },
+    ]);
+
     let mount_info = [l1, l2].join(&b"\n"[..]);
     MountInfo::parse(&mount_info).expect("failed to parse mount info file");
 }
-- 
2.20.1

Re: [pbs-devel] [PATCH] fix #2882: correctly parse optional fields from mountinfo

[Stefan Reiter]

Sorry, this is for the "proxmox" crate obviously, my clever patch script 
stripped the entire name as the prefix -.-

On 7/27/20 5:28 PM, Stefan Reiter wrote:
> As per the linux kernel documentation[0], the "optional fields" (called
> "tags" in our parser) are not comma-separated, but are a variable number
> (0 or more) of space-separated fields, always followed by a dash to mark
> the end.
> 
> Fix the /proc/<pid>/mountinfo parser to correctly parse these and add
> test cases (l3 is the line that produced the original warning from the
> bug report).
> 
> [0] https://www.kernel.org/doc/html/latest/filesystems/proc.html#proc-pid-mountinfo-information-about-mounts
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> I still think the way we parse here (with the fixed next()'s) is kind of weird,
> but I don't have a better idea and the rest of the "mountinfo" format looks
> pretty set in stone, so I suppose it's fine.
> 
>   proxmox/src/sys/linux/procfs/mountinfo.rs | 53 ++++++++++++++++++-----
>   1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/proxmox/src/sys/linux/procfs/mountinfo.rs b/proxmox/src/sys/linux/procfs/mountinfo.rs
> index 55f74c2..b57f132 100644
> --- a/proxmox/src/sys/linux/procfs/mountinfo.rs
> +++ b/proxmox/src/sys/linux/procfs/mountinfo.rs
> @@ -135,17 +135,18 @@ impl Entry {
>               root: OsStr::from_bytes(next()?).to_owned().into(),
>               mount_point: OsStr::from_bytes(next()?).to_owned().into(),
>               mount_options: OsStr::from_bytes(next()?).to_owned(),
> -            tags: next()?.split(|b| *b == b',').try_fold(
> -                Vec::new(),
> -                |mut acc, tag| -> Result<_, Error> {
> -                    acc.push(Tag::parse(tag)?);
> -                    Ok(acc)
> -                },
> -            )?,
> -            fs_type: std::str::from_utf8({
> -                next()?;
> -                next()?
> -            })?
> +            tags: {
> +                let mut tags = Vec::new();
> +                loop {
> +                    let tval = next()?;
> +                    if tval == b"-" {
> +                        break;
> +                    }
> +                    tags.push(Tag::parse(tval)?);
> +                }
> +                tags
> +            },
> +            fs_type: std::str::from_utf8(next()?)?
>               .to_string(),
>               mount_source: next().map(|src| match src {
>                   b"none" => None,
> @@ -350,6 +351,36 @@ fn test_entry() {
>           "rw,fd=26,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=27726"
>       );
>   
> +    // test different tag configurations
> +    let l3: &[u8] =
> +        b"225 224 0:46 / /proc rw,nosuid,nodev,noexec,relatime - proc proc rw";
> +    let entry = Entry::parse(l3).expect("failed to parse third mountinfo test entry");
> +    assert_eq!(entry.tags, &[]);
> +
> +    let l4: &[u8] =
> +        b"48 32 0:43 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime \
> +          shared:5 master:7 propagate_from:2 unbindable \
> +          - cgroup cgroup rw,blkio";
> +    let entry = Entry::parse(l4).expect("failed to parse fourth mountinfo test entry");
> +    assert_eq!(entry.tags, &[
> +        Tag {
> +            tag: OsString::from("shared"),
> +            value: Some(OsString::from("5")),
> +        },
> +        Tag {
> +            tag: OsString::from("master"),
> +            value: Some(OsString::from("7")),
> +        },
> +        Tag {
> +            tag: OsString::from("propagate_from"),
> +            value: Some(OsString::from("2")),
> +        },
> +        Tag {
> +            tag: OsString::from("unbindable"),
> +            value: None,
> +        },
> +    ]);
> +
>       let mount_info = [l1, l2].join(&b"\n"[..]);
>       MountInfo::parse(&mount_info).expect("failed to parse mount info file");
>   }
> 

[pbs-devel] applied: [PATCH] fix #2882: correctly parse optional fields from mountinfo

[Dietmar Maurer]

applied