mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Kaihang Zhang <kaihang.zhang@smartx.com>
To: Rich Felker <dalias@libc.org>
Cc: musl@lists.openwall.com, care <2010267516@qq.com>
Subject: Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r
Date: Tue, 18 Jan 2022 18:17:41 +0800	[thread overview]
Message-ID: <CAEVAO01=mdLq+xchM5ThOWPrHOaP+WxwxuStfiQQ+7voV92oJQ@mail.gmail.com> (raw)
In-Reply-To: <20220109031250.GN7074@brightrain.aerifal.cx>

On Sun, Jan 9, 2022 at 11:12 AM Rich Felker <dalias@libc.org> wrote:
>
> On Fri, Oct 15, 2021 at 08:20:00AM -0400, Kaihang Zhang wrote:
> > In function getmntent_r in source misc/mntent.c, entry that is too long
> > will be truncated rather than discarded. The caller can tell by errno
> > whether the supplied buffer is too small, and retry from the beginning
> > of the file.
> > ---
> >  src/misc/mntent.c | 53 +++++++++++++++++++++++++++++------------------
> >  1 file changed, 33 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> > index eabb8200..085ce45d 100644
> > --- a/src/misc/mntent.c
> > +++ b/src/misc/mntent.c
> > @@ -21,12 +21,12 @@ int endmntent(FILE *f)
> >
> >  struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int buflen)
> >  {
> > -     int cnt, n[8], use_internal = (linebuf == SENTINEL);
> > -
> > -     mnt->mnt_freq = 0;
> > -     mnt->mnt_passno = 0;
> > +     int use_internal = (linebuf == SENTINEL);
> > +     char *sub;
> >
> >       do {
> > +             char *end_ptr;
> > +
> >               if (use_internal) {
> >                       getline(&internal_buf, &internal_bufsize, f);
> >                       linebuf = internal_buf;
> > @@ -34,25 +34,38 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >                       fgets(linebuf, buflen, f);
> >               }
> >               if (feof(f) || ferror(f)) return 0;
> > -             if (!strchr(linebuf, '\n')) {
> > +
> > +             end_ptr = strchr(linebuf, '\n');
> > +             if (end_ptr != NULL) {
> > +                     while ((end_ptr[-1] == ' ' || end_ptr[-1] == '\t') && end_ptr != linebuf) end_ptr--;
> > +                     *end_ptr = '\0';
>
> Unless I'm misreading, this seems to invoke UB by reading the [-1]
> index before checking if that's valid. It could be fixed by just
> changing the order of the comparison expressions, but I'm not clear
> why this needs to be done anyway.
>
>
> > +             } else {
> >                       fscanf(f, "%*[^\n]%*[\n]");
> >                       errno = ERANGE;
> > -                     return 0;
> >               }
> > -             cnt = sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
> > -                     n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> > -                     &mnt->mnt_freq, &mnt->mnt_passno);
> > -     } while (cnt < 2 || linebuf[n[0]] == '#');
> > -
> > -     linebuf[n[1]] = 0;
> > -     linebuf[n[3]] = 0;
> > -     linebuf[n[5]] = 0;
> > -     linebuf[n[7]] = 0;
> > -
> > -     mnt->mnt_fsname = linebuf+n[0];
> > -     mnt->mnt_dir = linebuf+n[2];
> > -     mnt->mnt_type = linebuf+n[4];
> > -     mnt->mnt_opts = linebuf+n[6];
> > +
> > +             linebuf += strspn(linebuf, " \t");
> > +     } while (linebuf[0] == '\0' || linebuf[0] == '#');
> > +
> > +     mnt->mnt_fsname = strsep(&linebuf, " \t");
> > +
> > +     if (linebuf) linebuf += strspn(linebuf, " \t");
> > +     sub = strsep(&linebuf, " \t");
> > +     mnt->mnt_dir = sub ? sub : (char *) "";
>
> "" already has type char[1] and decays to char *; no cast is needed
> here.
>
> > +
> > +     if (linebuf) linebuf += strspn(linebuf, " \t");
> > +     sub = strsep (&linebuf, " \t");
> > +     mnt->mnt_type = sub ? sub : (char *) "";
> > +
> > +     if (linebuf) linebuf += strspn(linebuf, " \t");
> > +     sub = strsep(&linebuf, " \t");
> > +     mnt->mnt_opts = sub ? sub : (char *) "";
> > +
> > +     switch (linebuf ? sscanf(linebuf, " %d %d", &mnt->mnt_freq, &mnt->mnt_passno) : 0) {
> > +     case 0: mnt->mnt_freq = 0;
> > +     case 1: mnt->mnt_passno = 0;
> > +     case 2: break;
> > +     }
> >
> >       return mnt;
> >  }
> > --
> > 2.25.4
>
> This is gratuitously rewriting a lot of parsing logic in a form that
> doesn't seem like it's better, and even if it were, the change is
> orthogonal to fixing the behavior. I'm sorry for taking so long to get
> back to you and say this clearly.
>
> I do want to move forward on this because I know folks have been
> waiting on an upstream fix for a long time. But I need a patch
> accompanied by a clear explanation of the behavioral changes it's
> making, and just those changes. Or if we're in agreement on what the
> behavioral changes should be, I can just write the patch.
>
> The patch by Alyssa Ross is more minimal and better documented, but
> I'm not sure it covers everything you're concerned about. Could you
> let me know whether it does? I'll follow up on that thread about
> whether there are open issues with it.
>
> Rich

Thanks for taking time to check out this patch and give helpful suggestions!

I want to know if cgroup is enabled by using the information in
/proc/mounts, the contents are shown below (some unimportant entries
have been omitted).
---
> overlay / overlay rw,relatime,lowerdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/4083/fs,upperdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/4084/fs,workdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/4084/work 0 0
> cgroup /sys/fs/cgroup/systemd cgroup rw,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd 0 0
> cgroup /sys/fs/cgroup/cpu,cpuacct cgroup rw,nosuid,nodev,noexec,relatime,cpuacct,cpu 0 0
---

In glibc, we (at least I) usually use the following methods to get cgroup.
---
method1.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <mntent.h>
#include <errno.h>

void main() {
    struct mntent m;
    FILE *fd = NULL;
    char linebuf[256];

    fd = setmntent("/proc/mounts", "r");
    if (!fd) {
        printf("error: %s\n", strerror(errno));
        exit(-1);
    }

    while (getmntent_r(fd, &m, linebuf, sizeof(linebuf))) {
        if (!strcmp(m.mnt_type, "cgroup")) {
            printf("cgroup is enabled\n");
            exit(0);
        }
    }

    endmntent(fd);
}
---

The result was as I expected, cgroup is enabled, but  when using musl
libc, I got nothing! It's because when linebuf is small for the
too-long entry 'overlay', the function getmntent_r in musl libc will
return 0 and the entry 'cgroup' will not be read. Increasing  the
linebuf size will not help, beacause in a real scenario, the length of
the entry 'overlay' is unpredictable and could be 512 bytes, 1024
bytes, or longer.

I have to change the method1.c in musl libc.
---
method2.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <mntent.h>
#include <errno.h>

void main() {
    struct mntent m;
    FILE *fd = NULL;
    char linebuf[256];

    fd = setmntent("/proc/mounts", "r");
    if (!fd) {
        printf("error: %s\n", strerror(errno));
        exit(-1);
    }

again:
    errno = 0;
    while (getmntent_r(fd, &m, linebuf, sizeof(linebuf))) {
        if (!strcmp(m.mnt_type, "cgroup")) {
            printf("cgroup is enabled\n");
            exit(0);
        }
    }
    if (errno == ERANGE) {
        goto again;
    }

    endmntent(fd);
}
---

Or a more graceful way without 'goto'.
---
method3.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <mntent.h>
#include <errno.h>

void main() {
    struct mntent m;
    FILE *fd = NULL;
    char linebuf[256];

    fd = setmntent("/proc/mounts", "r");
    if (!fd) {
        printf("error: %s\n", strerror(errno));
        exit(-1);
    }

    for(;;) {
        errno = 0;
        if(getmntent_r(fd, &m, linebuf, sizeof(linebuf)) == NULL) {
            if (errno == ERANGE) {
                continue;
            }
            exit(0);
        }
        if (!strcmp(m.mnt_type, "cgroup")) {
            printf("cgroup is enabled\n");
            exit(0);
        }
    }

    endmntent(fd);
}
---

In musl libc, I have to look at errno to make sure I know if the
system has cgroup enabled. In fact, I'm sure linebuf is big enough to
hold mnt_fsname, mnt_dir, and mnt_type, and I can tell from mnt_type
whether the entry is a cgroup. Simply put, for a mount entry that is
too long, I want musl libc to truncate it rather than return 0
immediately, because I don't care about discarded contents or even the
entire mount entry that is too long. This way I don't have to pay
attention to errno to make sure I know if the system has cgroup
enabled. And I can use 'method1.c' in both glibc and musl libc to get
the result I want.

Errno is also necessary in some scenarios, such as when using the
hasmntopt function to ensure that too long mount entries are read to
linebuf.

Kaihang

--

      reply	other threads:[~2022-01-18 10:18 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-15 12:20 Kaihang Zhang
2021-12-01 12:33 ` [musl] " Kaihang Zhang
2021-12-01 15:24 ` [musl] " Rich Felker
2022-01-09  3:12 ` Rich Felker
2022-01-18 10:17   ` Kaihang Zhang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEVAO01=mdLq+xchM5ThOWPrHOaP+WxwxuStfiQQ+7voV92oJQ@mail.gmail.com' \
    --to=kaihang.zhang@smartx.com \
    --cc=2010267516@qq.com \
    --cc=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    --subject='Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).