[musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[Kaihang Zhang]

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';
+		} 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 *) "";
+
+	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

[musl] Re: [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[Kaihang Zhang]

The behavior of getmntent_r in glibc. If the buffer is too samll for a
line, the line will be truncated, glibc uses wrong data for this line.
However, there is no opportunity for the developer to realise a
mistake they made by supplying too small buffer, hence there is no
chance of recovering from it. The user gets the wrong data and doesn't
realize it.

The behavior of getmntent_r in musl libc. If the buffer is too small
for a line, it will set errno to ERANGE and then exit. And the user
can retry with a bigger buffer.
However, the current interface doesn't allow trying again to read the
too-long entry, since it will just have moved to the next one, the
too-long entry data will be lost. Maybe fseek(f, -fgets_result,
SEEK_CUR) could be used to rewind until the start of the line, but the
function would just loop eternally with too short a buffer.
For example, a mount entry has 2000 bytes, the size of buffer is 256
bytes, if errno is ERANGE i will retry use buffer size 512, 768, 1024,
1280, 1536, 1792, 2048... I have to try at least 8 times to get the
next mount entries.

In my opinion, it's ok to truncate the too-long entry (musl libc will
even lose it ! ) instead exiting. And the errno will be set to ERANGE
in order to let uesr realise this mistake, but as i know the user can
only retry from the beginning of the file, that bothers me somehow.
It‘s up to the user to decide to retry or not. If the user doesn't
care about the too-long entry or the discarded contents of it, there
is no need to retry. Othrewise, retry from the beginning of the file.


On Fri, Oct 15, 2021 at 8:20 PM Kaihang Zhang <kaihang.zhang@smartx.com> 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';
> +               } 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 *) "";
> +
> +       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
>


-- 
mail：kaihang.zhang@smartx.com
tell：15196469611

The SmartX email address is only for business purpose. Any sent
message that is not related to the business is not authorized or
permitted by SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.

Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[Rich Felker]

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.

Thanks for resending this. I know I've neglected to take a good look
at it so far, and I'll try to get to it right away this time.

Rich

Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[Rich Felker]

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

Re: [musl] [PATCH v2] fix: Truncate the too-long mntent in function getmntent_r

[Kaihang Zhang]

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

--