From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 23938 invoked from network); 18 Jan 2022 10:18:07 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 18 Jan 2022 10:18:07 -0000 Received: (qmail 13942 invoked by uid 550); 18 Jan 2022 10:18:05 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 13910 invoked from network); 18 Jan 2022 10:18:04 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=smartx-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=bhp1MH55BPh3uWu5On4Tb9ORMFW47ZS5EQBr8hIWMWQ=; b=Lksx9FO9qCAd24HhmThu48bcheEXSIUe6iv6bsbFxTjTmlLmf0tNITvb8tTUglevYM CxXsMWsM0S+pSbMqP30Nw+BpyMSMiXzeASWl2pni8p5F6fwzQSH3tqit720L5RdlDUEg XnsDzXu8WlEeb7Bn418xcafb9g6R2S0KqD1qP0SKAi4qHpH9TpHMWLRFB19QmBR6wsG2 2V2a4wjo3RrX0e6mfxt2BcDgzqZQfvcjjSAM3xf5CYdXMZhZ4Z2D/JHphRyOK0fKOfjT T8cXBeH52hpDOXgNsJ17qDcgImAUGElCJ5Pr9hY5g2U91QaR2Bo6LK5uBO4VZgfHHBrm wtlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=bhp1MH55BPh3uWu5On4Tb9ORMFW47ZS5EQBr8hIWMWQ=; b=PX0roKdhpGvz8xBfV5C6B/17O120R7imPU86G/CJpC/p/sJ/kvoSY7jXCu99KrsevG NzraBhD0wbhqqBz8sAnxs+xAIs/6s3rq5xhGm5lGvvig30X82N27zj/MpYZgMSrpadYf QOVpiCDLvHWVscrLeKUnQkg2QDrpMq3PDe9/yobq1O5A9naURKiAnP9LdTkkGwitJLDZ GVdZbMkvU5pqALWNnpPsTGegXeDW/a6n/BT71gKTDi7J7QSxxwwHWJ2t81ZvY/JM9TdG ONXCymb52pCtup4aqLc3nvAogvOdQaq83NFZmEIkoy8IcHQ+D/vPWwdR1d0kKy+H9DYP rTDQ== X-Gm-Message-State: AOAM532B0KzHsf6QOwL7qDHiJ2iofwknBOMR1iv5sBmyj8aSgfELVY7V XOHw8FofbqOBZ+aMvgTsh5gL2fjIuDaIj/9E9t5BNw== X-Google-Smtp-Source: ABdhPJwAaddSwaoCdWvEpdyaGuBnmG3DHxaDeFWR/C+AnFyFbUgHMpakcC+WbKhYWSJu7Q73V+WI4aYFPhln+r0aGvM= X-Received: by 2002:a05:6512:695:: with SMTP id t21mr15652156lfe.132.1642501072810; Tue, 18 Jan 2022 02:17:52 -0800 (PST) MIME-Version: 1.0 References: <20211015122000.2490-1-kaihang.zhang@smartx.com> <20220109031250.GN7074@brightrain.aerifal.cx> In-Reply-To: <20220109031250.GN7074@brightrain.aerifal.cx> From: Kaihang Zhang Date: Tue, 18 Jan 2022 18:17:41 +0800 Message-ID: To: Rich Felker Cc: musl@lists.openwall.com, care <2010267516@qq.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r On Sun, Jan 9, 2022 at 11:12 AM Rich Felker 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 =3D (linebuf =3D=3D SENTINEL); > > - > > - mnt->mnt_freq =3D 0; > > - mnt->mnt_passno =3D 0; > > + int use_internal =3D (linebuf =3D=3D SENTINEL); > > + char *sub; > > > > do { > > + char *end_ptr; > > + > > if (use_internal) { > > getline(&internal_buf, &internal_bufsize, f); > > linebuf =3D 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 =3D strchr(linebuf, '\n'); > > + if (end_ptr !=3D NULL) { > > + while ((end_ptr[-1] =3D=3D ' ' || end_ptr[-1] =3D= =3D '\t') && end_ptr !=3D linebuf) end_ptr--; > > + *end_ptr =3D '\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 =3D ERANGE; > > - return 0; > > } > > - cnt =3D 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]] =3D=3D '#'); > > - > > - linebuf[n[1]] =3D 0; > > - linebuf[n[3]] =3D 0; > > - linebuf[n[5]] =3D 0; > > - linebuf[n[7]] =3D 0; > > - > > - mnt->mnt_fsname =3D linebuf+n[0]; > > - mnt->mnt_dir =3D linebuf+n[2]; > > - mnt->mnt_type =3D linebuf+n[4]; > > - mnt->mnt_opts =3D linebuf+n[6]; > > + > > + linebuf +=3D strspn(linebuf, " \t"); > > + } while (linebuf[0] =3D=3D '\0' || linebuf[0] =3D=3D '#'); > > + > > + mnt->mnt_fsname =3D strsep(&linebuf, " \t"); > > + > > + if (linebuf) linebuf +=3D strspn(linebuf, " \t"); > > + sub =3D strsep(&linebuf, " \t"); > > + mnt->mnt_dir =3D sub ? sub : (char *) ""; > > "" already has type char[1] and decays to char *; no cast is needed > here. > > > + > > + if (linebuf) linebuf +=3D strspn(linebuf, " \t"); > > + sub =3D strsep (&linebuf, " \t"); > > + mnt->mnt_type =3D sub ? sub : (char *) ""; > > + > > + if (linebuf) linebuf +=3D strspn(linebuf, " \t"); > > + sub =3D strsep(&linebuf, " \t"); > > + mnt->mnt_opts =3D sub ? sub : (char *) ""; > > + > > + switch (linebuf ? sscanf(linebuf, " %d %d", &mnt->mnt_freq, &mnt-= >mnt_passno) : 0) { > > + case 0: mnt->mnt_freq =3D 0; > > + case 1: mnt->mnt_passno =3D 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=3D/var/lib/containerd/io.container= d.snapshotter.v1.overlayfs/snapshots/4083/fs,upperdir=3D/var/lib/containerd= /io.containerd.snapshotter.v1.overlayfs/snapshots/4084/fs,workdir=3D/var/li= b/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/4084/work 0 0 > cgroup /sys/fs/cgroup/systemd cgroup rw,nosuid,nodev,noexec,relatime,xatt= r,release_agent=3D/usr/lib/systemd/systemd-cgroups-agent,name=3Dsystemd 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 #include #include #include #include void main() { struct mntent m; FILE *fd =3D NULL; char linebuf[256]; fd =3D 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 #include #include #include #include void main() { struct mntent m; FILE *fd =3D NULL; char linebuf[256]; fd =3D setmntent("/proc/mounts", "r"); if (!fd) { printf("error: %s\n", strerror(errno)); exit(-1); } again: errno =3D 0; while (getmntent_r(fd, &m, linebuf, sizeof(linebuf))) { if (!strcmp(m.mnt_type, "cgroup")) { printf("cgroup is enabled\n"); exit(0); } } if (errno =3D=3D ERANGE) { goto again; } endmntent(fd); } --- Or a more graceful way without 'goto'. --- method3.c #include #include #include #include #include void main() { struct mntent m; FILE *fd =3D NULL; char linebuf[256]; fd =3D setmntent("/proc/mounts", "r"); if (!fd) { printf("error: %s\n", strerror(errno)); exit(-1); } for(;;) { errno =3D 0; if(getmntent_r(fd, &m, linebuf, sizeof(linebuf)) =3D=3D NULL) { if (errno =3D=3D 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 --