[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Tue, 18 Jan 2022 18:17:41 +0800
From: Kaihang Zhang <kaihang.zhang@...rtx.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com, care <2010267516@...com>
Subject: Re: [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r
On Sun, Jan 9, 2022 at 11:12 AM Rich Felker <dalias@...c.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
--
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
