mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH v2 0/3]
@ 2021-09-15 22:11 Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH libc-test v2 1/3] functional: add mntent test Alyssa Ross
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Alyssa Ross @ 2021-09-15 22:11 UTC (permalink / raw)
  To: musl; +Cc: Érico Nogueira, Rich Felker

v2: don't change n from int to size_t
v1: https://inbox.vuxu.org/musl/20210821085420.474615-1-hi@alyssa.is/

This series introduces tests for libc-test that demonstrate a Musl
bug, and follows them with a fix for that bug.

-- 
2.32.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [musl] [PATCH libc-test v2 1/3] functional: add mntent test
  2021-09-15 22:11 [musl] [PATCH v2 0/3] Alyssa Ross
@ 2021-09-15 22:11 ` Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH libc-test v2 2/3] functional: add mntent test for single-field line Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields Alyssa Ross
  2 siblings, 0 replies; 22+ messages in thread
From: Alyssa Ross @ 2021-09-15 22:11 UTC (permalink / raw)
  To: musl; +Cc: Érico Nogueira, Rich Felker

This only checks reading an fstab from an stream.  I haven't written
tests for either setmntent(), addmntent(), or hasmntnent().

test_getmntent exposes a bug in Musl where lines omitting the final
two fields, which are supposed to be optional according to fstab(5),
are not accepted.  The tests all pass on Glibc.
---
 AUTHORS                 |  1 +
 src/functional/mntent.c | 76 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 src/functional/mntent.c

diff --git a/AUTHORS b/AUTHORS
index ff99471..cf2a394 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -5,3 +5,4 @@ John Spencer
 Jens Gustedt
 Alexander Monakov
 Julien Ramseier
+Alyssa Ross
diff --git a/src/functional/mntent.c b/src/functional/mntent.c
new file mode 100644
index 0000000..59d816a
--- /dev/null
+++ b/src/functional/mntent.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: MIT
+
+#define _DEFAULT_SOURCE // for getmntent_r
+
+#include <mntent.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+
+#include "test.h"
+
+#define ASSERT(x) do {				 \
+		if (!(x)) {			 \
+			t_error(#x " failed\n"); \
+			exit(EXIT_FAILURE);	 \
+		}				 \
+	} while (0);
+
+#define ERR(fmt, ...) do {					       \
+		t_error(fmt ": %s\n", ##__VA_ARGS__, strerror(errno)); \
+		exit(EXIT_FAILURE);				       \
+	} while (0)
+
+void test_getmntent_empty(void)
+{
+	char fstab[] = "\n";
+	FILE *f = fmemopen((void *)fstab, sizeof fstab - 1, "r");
+	if (!f) ERR("fmemopen");
+	ASSERT(!getmntent(f));
+	ASSERT(endmntent(f) == 1);
+}
+
+void test_getmntent(void)
+{
+	// Checks that the fifth and sixth fields default to 0.
+	char fstab[] = "none /proc proc defaults\n";
+	FILE *f = fmemopen((void *)fstab, sizeof fstab - 1, "r");
+	if (!f) ERR("fmemopen");
+	struct mntent *m = getmntent(f);
+	ASSERT(m);
+	ASSERT(!strcmp(m->mnt_fsname, "none"));
+	ASSERT(!strcmp(m->mnt_dir, "/proc"));
+	ASSERT(!strcmp(m->mnt_type, "proc"));
+	ASSERT(!strcmp(m->mnt_opts, "defaults"));
+	ASSERT(m->mnt_freq == 0);
+	ASSERT(m->mnt_passno == 0);
+	ASSERT(endmntent(f) == 1);
+}
+
+void test_getmntent_r(void)
+{
+	struct mntent m, *r;
+	char fstab[] = "/dev/sda\t/\text4\trw,nosuid\t2\t1\n";
+	char buf[sizeof(fstab)];
+
+	FILE *f = fmemopen((void *)fstab, sizeof fstab - 1, "r");
+	if (!f) ERR("fmemopen");
+
+	r = getmntent_r(f, &m, buf, sizeof buf);
+	ASSERT(r == &m);
+	ASSERT(!strcmp(m.mnt_fsname, "/dev/sda"));
+	ASSERT(!strcmp(m.mnt_dir, "/"));
+	ASSERT(!strcmp(m.mnt_type, "ext4"));
+	ASSERT(!strcmp(m.mnt_opts, "rw,nosuid"));
+	ASSERT(m.mnt_freq == 2);
+	ASSERT(m.mnt_passno == 1);
+	ASSERT(endmntent(f) == 1);
+}
+
+int main(void)
+{
+	test_getmntent_empty();
+	test_getmntent();
+	test_getmntent_r();
+}
-- 
2.32.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [musl] [PATCH libc-test v2 2/3] functional: add mntent test for single-field line
  2021-09-15 22:11 [musl] [PATCH v2 0/3] Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH libc-test v2 1/3] functional: add mntent test Alyssa Ross
@ 2021-09-15 22:11 ` Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields Alyssa Ross
  2 siblings, 0 replies; 22+ messages in thread
From: Alyssa Ross @ 2021-09-15 22:11 UTC (permalink / raw)
  To: musl; +Cc: Érico Nogueira, Rich Felker

Glibc only requires a single field to be present in an fstab line to
parse it, and will initialize all other string fields to the empty
string.  This test checks for that behaviour.
---

I'm providing this test as a seperate patch to make it easy to pass on
this test while still accepting the rest, because I'm not sure whether
it's a good thing or not for Musl to allow fstab lines like this, even
though Glibc does.

 src/functional/mntent.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/functional/mntent.c b/src/functional/mntent.c
index 59d816a..caa7d33 100644
--- a/src/functional/mntent.c
+++ b/src/functional/mntent.c
@@ -31,6 +31,18 @@ void test_getmntent_empty(void)
 	ASSERT(endmntent(f) == 1);
 }
 
+void test_getmntent_short(void)
+{
+	char fstab[] = "1\n";
+	FILE *f = fmemopen((void *)fstab, sizeof fstab - 1, "r");
+	if (!f) ERR("fmemopen");
+	struct mntent *m = getmntent(f);
+	ASSERT(m);
+	ASSERT(!strcmp(m->mnt_fsname, "1"));
+	ASSERT(!*m->mnt_dir);
+	ASSERT(endmntent(f) == 1);
+}
+
 void test_getmntent(void)
 {
 	// Checks that the fifth and sixth fields default to 0.
@@ -71,6 +83,7 @@ void test_getmntent_r(void)
 int main(void)
 {
 	test_getmntent_empty();
+	test_getmntent_short();
 	test_getmntent();
 	test_getmntent_r();
 }
-- 
2.32.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2021-09-15 22:11 [musl] [PATCH v2 0/3] Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH libc-test v2 1/3] functional: add mntent test Alyssa Ross
  2021-09-15 22:11 ` [musl] [PATCH libc-test v2 2/3] functional: add mntent test for single-field line Alyssa Ross
@ 2021-09-15 22:11 ` Alyssa Ross
  2021-09-20  4:21   ` Rich Felker
  2 siblings, 1 reply; 22+ messages in thread
From: Alyssa Ross @ 2021-09-15 22:11 UTC (permalink / raw)
  To: musl; +Cc: Érico Nogueira, Rich Felker

According to fstab(5), the last two fields are optional, but this
wasn't accepted by Musl.  After this change, only the first field is
required, which matches Glibc's behaviour.

Using sscanf as before, it would have been impossible to differentiate
between 0 fields and 4 fields, because sscanf would have returned 0 in
both cases due to the use of assignment suppression and %n for the
string fields (which is important to avoid copying any strings).  So
instead, before calling sscanf, initialize every string to the empty
string, and then we can check which strings are empty afterwards to
know how many fields were matched.
---

We could also be stricter about it, and enforce that the first four
fields are present, since the man page says only the last two are
optional.  Doing that would be a simple change of checking for the
presence of mnt_opts instead of mnt_fsname at the end of my patch.

v2: don't change n from int to size_t

 src/misc/mntent.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/misc/mntent.c b/src/misc/mntent.c
index eabb8200..238a0efd 100644
--- a/src/misc/mntent.c
+++ b/src/misc/mntent.c
@@ -21,7 +21,8 @@ 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);
+	int n[8], use_internal = (linebuf == SENTINEL);
+	size_t len, i;
 
 	mnt->mnt_freq = 0;
 	mnt->mnt_passno = 0;
@@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
 			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]] == '#');
+
+		len = strlen(linebuf);
+		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
+		if (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) == EOF && ferror(f))
+			return 0;
+	} while (linebuf[n[0]] == '#');
 
 	linebuf[n[1]] = 0;
 	linebuf[n[3]] = 0;
@@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
 	mnt->mnt_type = linebuf+n[4];
 	mnt->mnt_opts = linebuf+n[6];
 
+	if (!*mnt->mnt_fsname)
+		return 0;
+
 	return mnt;
 }
 
-- 
2.32.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2021-09-15 22:11 ` [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields Alyssa Ross
@ 2021-09-20  4:21   ` Rich Felker
  2022-01-09  3:18     ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2021-09-20  4:21 UTC (permalink / raw)
  To: musl; +Cc: Alyssa Ross

On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> According to fstab(5), the last two fields are optional, but this
> wasn't accepted by Musl.  After this change, only the first field is
> required, which matches Glibc's behaviour.
> 
> Using sscanf as before, it would have been impossible to differentiate
> between 0 fields and 4 fields, because sscanf would have returned 0 in
> both cases due to the use of assignment suppression and %n for the
> string fields (which is important to avoid copying any strings).  So
> instead, before calling sscanf, initialize every string to the empty
> string, and then we can check which strings are empty afterwards to
> know how many fields were matched.
> ---
> 
> We could also be stricter about it, and enforce that the first four
> fields are present, since the man page says only the last two are
> optional.  Doing that would be a simple change of checking for the
> presence of mnt_opts instead of mnt_fsname at the end of my patch.
> 
> v2: don't change n from int to size_t
> 
>  src/misc/mntent.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> index eabb8200..238a0efd 100644
> --- a/src/misc/mntent.c
> +++ b/src/misc/mntent.c
> @@ -21,7 +21,8 @@ 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);
> +	int n[8], use_internal = (linebuf == SENTINEL);
> +	size_t len, i;
>  
>  	mnt->mnt_freq = 0;
>  	mnt->mnt_passno = 0;
> @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>  			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]] == '#');
> +
> +		len = strlen(linebuf);
> +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> +		if (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) == EOF && ferror(f))
> +			return 0;
> +	} while (linebuf[n[0]] == '#');
>  
>  	linebuf[n[1]] = 0;
>  	linebuf[n[3]] = 0;
> @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>  	mnt->mnt_type = linebuf+n[4];
>  	mnt->mnt_opts = linebuf+n[6];
>  
> +	if (!*mnt->mnt_fsname)
> +		return 0;
> +
>  	return mnt;
>  }

It looks like your patch changes the behavior for malformed lines from
skipping them (and continuing to search for the next valid line) to
returning 0. Is that intentional? Maybe it's better; I'm not sure. But
won't it even cause blank lines to return 0?

A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
char* argument to collect the value before the " %d %d". Then you can
check for cnt<1. But I'm not sure even the 4th field should be
mandatory. This same apprach could be used to make just 3 mandatory if
desired though.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2021-09-20  4:21   ` Rich Felker
@ 2022-01-09  3:18     ` Rich Felker
  2022-01-13 16:30       ` Alyssa Ross
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-01-09  3:18 UTC (permalink / raw)
  To: musl; +Cc: Alyssa Ross

On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> > According to fstab(5), the last two fields are optional, but this
> > wasn't accepted by Musl.  After this change, only the first field is
> > required, which matches Glibc's behaviour.
> > 
> > Using sscanf as before, it would have been impossible to differentiate
> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> > both cases due to the use of assignment suppression and %n for the
> > string fields (which is important to avoid copying any strings).  So
> > instead, before calling sscanf, initialize every string to the empty
> > string, and then we can check which strings are empty afterwards to
> > know how many fields were matched.
> > ---
> > 
> > We could also be stricter about it, and enforce that the first four
> > fields are present, since the man page says only the last two are
> > optional.  Doing that would be a simple change of checking for the
> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > 
> > v2: don't change n from int to size_t
> > 
> >  src/misc/mntent.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> > index eabb8200..238a0efd 100644
> > --- a/src/misc/mntent.c
> > +++ b/src/misc/mntent.c
> > @@ -21,7 +21,8 @@ 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);
> > +	int n[8], use_internal = (linebuf == SENTINEL);
> > +	size_t len, i;
> >  
> >  	mnt->mnt_freq = 0;
> >  	mnt->mnt_passno = 0;
> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >  			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]] == '#');
> > +
> > +		len = strlen(linebuf);
> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> > +		if (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) == EOF && ferror(f))
> > +			return 0;
> > +	} while (linebuf[n[0]] == '#');
> >  
> >  	linebuf[n[1]] = 0;
> >  	linebuf[n[3]] = 0;
> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >  	mnt->mnt_type = linebuf+n[4];
> >  	mnt->mnt_opts = linebuf+n[6];
> >  
> > +	if (!*mnt->mnt_fsname)
> > +		return 0;
> > +
> >  	return mnt;
> >  }
> 
> It looks like your patch changes the behavior for malformed lines from
> skipping them (and continuing to search for the next valid line) to
> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> won't it even cause blank lines to return 0?

Indeed it also seems to be skipping empty lines, contrary to what you
said in another message:

>  • Empty lines should be skipped.

Do you have a preference on how to proceed? We could add back a
condition to the while loop, something like linebuf[n[0]]=='#' ||
n[6]==len (i.e. skip lines with too few fields, possibly using a
different number instead of 6 if more appropriate). Or we could do
what I suggested before:

> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> char* argument to collect the value before the " %d %d". Then you can
> check for cnt<1. But I'm not sure even the 4th field should be
> mandatory. This same apprach could be used to make just 3 mandatory if
> desired though.

Thoughts?

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-01-09  3:18     ` Rich Felker
@ 2022-01-13 16:30       ` Alyssa Ross
  2022-01-13 17:40         ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Alyssa Ross @ 2022-01-13 16:30 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 7016 bytes --]

Hi Rich, thanks for following up on this.

Rich Felker <dalias@libc.org> writes:

> On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
>> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
>> > According to fstab(5), the last two fields are optional, but this
>> > wasn't accepted by Musl.  After this change, only the first field is
>> > required, which matches Glibc's behaviour.
>> > 
>> > Using sscanf as before, it would have been impossible to differentiate
>> > between 0 fields and 4 fields, because sscanf would have returned 0 in
>> > both cases due to the use of assignment suppression and %n for the
>> > string fields (which is important to avoid copying any strings).  So
>> > instead, before calling sscanf, initialize every string to the empty
>> > string, and then we can check which strings are empty afterwards to
>> > know how many fields were matched.
>> > ---
>> > 
>> > We could also be stricter about it, and enforce that the first four
>> > fields are present, since the man page says only the last two are
>> > optional.  Doing that would be a simple change of checking for the
>> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
>> > 
>> > v2: don't change n from int to size_t
>> > 
>> >  src/misc/mntent.c | 18 +++++++++++++-----
>> >  1 file changed, 13 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
>> > index eabb8200..238a0efd 100644
>> > --- a/src/misc/mntent.c
>> > +++ b/src/misc/mntent.c
>> > @@ -21,7 +21,8 @@ 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);
>> > +	int n[8], use_internal = (linebuf == SENTINEL);
>> > +	size_t len, i;
>> >  
>> >  	mnt->mnt_freq = 0;
>> >  	mnt->mnt_passno = 0;
>> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>> >  			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]] == '#');
>> > +
>> > +		len = strlen(linebuf);
>> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
>> > +		if (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) == EOF && ferror(f))
>> > +			return 0;
>> > +	} while (linebuf[n[0]] == '#');
>> >  
>> >  	linebuf[n[1]] = 0;
>> >  	linebuf[n[3]] = 0;
>> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>> >  	mnt->mnt_type = linebuf+n[4];
>> >  	mnt->mnt_opts = linebuf+n[6];
>> >  
>> > +	if (!*mnt->mnt_fsname)
>> > +		return 0;
>> > +
>> >  	return mnt;
>> >  }
>> 
>> It looks like your patch changes the behavior for malformed lines from
>> skipping them (and continuing to search for the next valid line) to
>> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
>> won't it even cause blank lines to return 0?

Because I only check for the first field being present, a lot of
nonsensical lines will be accepted.

As I said in the patch commentary:

> After this change, only the first field is
> required, which matches Glibc's behaviour.
>
> [snip]
>
> We could also be stricter about it, and enforce that the first four
> fields are present, since the man page says only the last two are
> optional.  Doing that would be a simple change of checking for the
> presence of mnt_opts instead of mnt_fsname at the end of my patch.

It probably would make more sense to check that the four fields the man
pages implies are required are all there, by making the change I
suggested in the commentary, at least until somebody complains about
their two-field fstab being accepted by Glibc and not Musl.

> Indeed it also seems to be skipping empty lines, contrary to what you
> said in another message:
>
>>  • Empty lines should be skipped.

Yes, it looks like I was mistaken before when I thought that Musl didn't
properly handle comments and empty lines.  Looking back, it seems that
the tests I was running were against mntent files with only four fields,
so the parsing failures I was seeing were because of that, not because
of an issue in the comment or empty line handling.

My patch does (inadventently) change the behaviour of empty line
handling.  We should leave the current behaviour of skipping over empty
lines as is.  Suggested fix at the end of this message.

> Do you have a preference on how to proceed? We could add back a
> condition to the while loop, something like linebuf[n[0]]=='#' ||
> n[6]==len (i.e. skip lines with too few fields, possibly using a
> different number instead of 6 if more appropriate). Or we could do
> what I suggested before:
>
>> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
>> char* argument to collect the value before the " %d %d". Then you can
>> check for cnt<1. But I'm not sure even the 4th field should be
>> mandatory. This same apprach could be used to make just 3 mandatory if
>> desired though.
>
> Thoughts?

I think it would clearer to have an explicit check that the last
mandatory field is set, like I currently do with the mnt_fsname check at
the end of the function.  I don't particularly mind how many fields are
mandatory, as long as its four or fewer so Musl's behaviour follows the
fstab format described in the man page.

So overall my proposed revisions would be the following.  There's a
change to move to the next line if the current one is empty, and a
change to ensure the first four fields are all present.  (If you decide
you'd like the fourth field to also be optional, we can just change
mnt_opts to mnt_type in that check.)  It's been a long time since I last
this code, btw, so I hope I'm not missing anything around the empty line
check.  I'd be happy to put together a revised series, with these
changes and a corresponding change to my libc-test patch, if you'd like.

diff --git i/src/misc/mntent.c w/src/misc/mntent.c
index 169e9789..7782cb10 100644
--- i/src/misc/mntent.c
+++ w/src/misc/mntent.c
@@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
 			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
 			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
 			return 0;
-	} while (linebuf[n[0]] == '#');
+	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
 
 	linebuf[n[1]] = 0;
 	linebuf[n[3]] = 0;
@@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
 	mnt->mnt_type = linebuf+n[4];
 	mnt->mnt_opts = linebuf+n[6];
 
-	if (!*mnt->mnt_fsname)
+	if (!*mnt->mnt_opts)
 		return 0;
 
 	return mnt;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-01-13 16:30       ` Alyssa Ross
@ 2022-01-13 17:40         ` Rich Felker
  2022-01-13 18:53           ` Alyssa Ross
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-01-13 17:40 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: musl

On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
> Hi Rich, thanks for following up on this.
> 
> Rich Felker <dalias@libc.org> writes:
> 
> > On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> >> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> >> > According to fstab(5), the last two fields are optional, but this
> >> > wasn't accepted by Musl.  After this change, only the first field is
> >> > required, which matches Glibc's behaviour.
> >> > 
> >> > Using sscanf as before, it would have been impossible to differentiate
> >> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> >> > both cases due to the use of assignment suppression and %n for the
> >> > string fields (which is important to avoid copying any strings).  So
> >> > instead, before calling sscanf, initialize every string to the empty
> >> > string, and then we can check which strings are empty afterwards to
> >> > know how many fields were matched.
> >> > ---
> >> > 
> >> > We could also be stricter about it, and enforce that the first four
> >> > fields are present, since the man page says only the last two are
> >> > optional.  Doing that would be a simple change of checking for the
> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> >> > 
> >> > v2: don't change n from int to size_t
> >> > 
> >> >  src/misc/mntent.c | 18 +++++++++++++-----
> >> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >> > 
> >> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> >> > index eabb8200..238a0efd 100644
> >> > --- a/src/misc/mntent.c
> >> > +++ b/src/misc/mntent.c
> >> > @@ -21,7 +21,8 @@ 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);
> >> > +	int n[8], use_internal = (linebuf == SENTINEL);
> >> > +	size_t len, i;
> >> >  
> >> >  	mnt->mnt_freq = 0;
> >> >  	mnt->mnt_passno = 0;
> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >> >  			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]] == '#');
> >> > +
> >> > +		len = strlen(linebuf);
> >> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> >> > +		if (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) == EOF && ferror(f))
> >> > +			return 0;
> >> > +	} while (linebuf[n[0]] == '#');
> >> >  
> >> >  	linebuf[n[1]] = 0;
> >> >  	linebuf[n[3]] = 0;
> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >> >  	mnt->mnt_type = linebuf+n[4];
> >> >  	mnt->mnt_opts = linebuf+n[6];
> >> >  
> >> > +	if (!*mnt->mnt_fsname)
> >> > +		return 0;
> >> > +
> >> >  	return mnt;
> >> >  }
> >> 
> >> It looks like your patch changes the behavior for malformed lines from
> >> skipping them (and continuing to search for the next valid line) to
> >> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> >> won't it even cause blank lines to return 0?
> 
> Because I only check for the first field being present, a lot of
> nonsensical lines will be accepted.
> 
> As I said in the patch commentary:
> 
> > After this change, only the first field is
> > required, which matches Glibc's behaviour.
> >
> > [snip]
> >
> > We could also be stricter about it, and enforce that the first four
> > fields are present, since the man page says only the last two are
> > optional.  Doing that would be a simple change of checking for the
> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> 
> It probably would make more sense to check that the four fields the man
> pages implies are required are all there, by making the change I
> suggested in the commentary, at least until somebody complains about
> their two-field fstab being accepted by Glibc and not Musl.
> 
> > Indeed it also seems to be skipping empty lines, contrary to what you
> > said in another message:
> >
> >>  • Empty lines should be skipped.
> 
> Yes, it looks like I was mistaken before when I thought that Musl didn't
> properly handle comments and empty lines.  Looking back, it seems that
> the tests I was running were against mntent files with only four fields,
> so the parsing failures I was seeing were because of that, not because
> of an issue in the comment or empty line handling.
> 
> My patch does (inadventently) change the behaviour of empty line
> handling.  We should leave the current behaviour of skipping over empty
> lines as is.  Suggested fix at the end of this message.
> 
> > Do you have a preference on how to proceed? We could add back a
> > condition to the while loop, something like linebuf[n[0]]=='#' ||
> > n[6]==len (i.e. skip lines with too few fields, possibly using a
> > different number instead of 6 if more appropriate). Or we could do
> > what I suggested before:
> >
> >> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> >> char* argument to collect the value before the " %d %d". Then you can
> >> check for cnt<1. But I'm not sure even the 4th field should be
> >> mandatory. This same apprach could be used to make just 3 mandatory if
> >> desired though.
> >
> > Thoughts?
> 
> I think it would clearer to have an explicit check that the last
> mandatory field is set, like I currently do with the mnt_fsname check at
> the end of the function.  I don't particularly mind how many fields are
> mandatory, as long as its four or fewer so Musl's behaviour follows the
> fstab format described in the man page.
> 
> So overall my proposed revisions would be the following.  There's a
> change to move to the next line if the current one is empty, and a
> change to ensure the first four fields are all present.  (If you decide
> you'd like the fourth field to also be optional, we can just change
> mnt_opts to mnt_type in that check.)  It's been a long time since I last
> this code, btw, so I hope I'm not missing anything around the empty line
> check.  I'd be happy to put together a revised series, with these
> changes and a corresponding change to my libc-test patch, if you'd like.
> 
> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
> index 169e9789..7782cb10 100644
> --- i/src/misc/mntent.c
> +++ w/src/misc/mntent.c
> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>  			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
>  			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
>  			return 0;
> -	} while (linebuf[n[0]] == '#');
> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
>  
>  	linebuf[n[1]] = 0;
>  	linebuf[n[3]] = 0;
> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>  	mnt->mnt_type = linebuf+n[4];
>  	mnt->mnt_opts = linebuf+n[6];
>  
> -	if (!*mnt->mnt_fsname)
> +	if (!*mnt->mnt_opts)
>  		return 0;
>  
>  	return mnt;

What I still don't like here is that this changes the behavior on
something that's not a valid record (in whatever sense we define that)
from continuing the loop looking for the next one, to returning a null
pointer with no indication of what the error was. Treating empty lines
as an error (rather than continuing) was just one special case of
that. For an analogy, see how the pwd/grp functions work. Something
malformed in the file is just "not a record" and search continues for
the next valid record (if any) rather than giving the caller a
non-actionable (since this is assumed to be a sort of
trusted/authoritative data outside the application's control) error.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-01-13 17:40         ` Rich Felker
@ 2022-01-13 18:53           ` Alyssa Ross
  2022-05-12 14:08             ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Alyssa Ross @ 2022-01-13 18:53 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 8968 bytes --]

Rich Felker <dalias@libc.org> writes:

> On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
>> Hi Rich, thanks for following up on this.
>> 
>> Rich Felker <dalias@libc.org> writes:
>> 
>> > On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
>> >> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
>> >> > According to fstab(5), the last two fields are optional, but this
>> >> > wasn't accepted by Musl.  After this change, only the first field is
>> >> > required, which matches Glibc's behaviour.
>> >> > 
>> >> > Using sscanf as before, it would have been impossible to differentiate
>> >> > between 0 fields and 4 fields, because sscanf would have returned 0 in
>> >> > both cases due to the use of assignment suppression and %n for the
>> >> > string fields (which is important to avoid copying any strings).  So
>> >> > instead, before calling sscanf, initialize every string to the empty
>> >> > string, and then we can check which strings are empty afterwards to
>> >> > know how many fields were matched.
>> >> > ---
>> >> > 
>> >> > We could also be stricter about it, and enforce that the first four
>> >> > fields are present, since the man page says only the last two are
>> >> > optional.  Doing that would be a simple change of checking for the
>> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
>> >> > 
>> >> > v2: don't change n from int to size_t
>> >> > 
>> >> >  src/misc/mntent.c | 18 +++++++++++++-----
>> >> >  1 file changed, 13 insertions(+), 5 deletions(-)
>> >> > 
>> >> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
>> >> > index eabb8200..238a0efd 100644
>> >> > --- a/src/misc/mntent.c
>> >> > +++ b/src/misc/mntent.c
>> >> > @@ -21,7 +21,8 @@ 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);
>> >> > +	int n[8], use_internal = (linebuf == SENTINEL);
>> >> > +	size_t len, i;
>> >> >  
>> >> >  	mnt->mnt_freq = 0;
>> >> >  	mnt->mnt_passno = 0;
>> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>> >> >  			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]] == '#');
>> >> > +
>> >> > +		len = strlen(linebuf);
>> >> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
>> >> > +		if (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) == EOF && ferror(f))
>> >> > +			return 0;
>> >> > +	} while (linebuf[n[0]] == '#');
>> >> >  
>> >> >  	linebuf[n[1]] = 0;
>> >> >  	linebuf[n[3]] = 0;
>> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>> >> >  	mnt->mnt_type = linebuf+n[4];
>> >> >  	mnt->mnt_opts = linebuf+n[6];
>> >> >  
>> >> > +	if (!*mnt->mnt_fsname)
>> >> > +		return 0;
>> >> > +
>> >> >  	return mnt;
>> >> >  }
>> >> 
>> >> It looks like your patch changes the behavior for malformed lines from
>> >> skipping them (and continuing to search for the next valid line) to
>> >> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
>> >> won't it even cause blank lines to return 0?
>> 
>> Because I only check for the first field being present, a lot of
>> nonsensical lines will be accepted.
>> 
>> As I said in the patch commentary:
>> 
>> > After this change, only the first field is
>> > required, which matches Glibc's behaviour.
>> >
>> > [snip]
>> >
>> > We could also be stricter about it, and enforce that the first four
>> > fields are present, since the man page says only the last two are
>> > optional.  Doing that would be a simple change of checking for the
>> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
>> 
>> It probably would make more sense to check that the four fields the man
>> pages implies are required are all there, by making the change I
>> suggested in the commentary, at least until somebody complains about
>> their two-field fstab being accepted by Glibc and not Musl.
>> 
>> > Indeed it also seems to be skipping empty lines, contrary to what you
>> > said in another message:
>> >
>> >>  • Empty lines should be skipped.
>> 
>> Yes, it looks like I was mistaken before when I thought that Musl didn't
>> properly handle comments and empty lines.  Looking back, it seems that
>> the tests I was running were against mntent files with only four fields,
>> so the parsing failures I was seeing were because of that, not because
>> of an issue in the comment or empty line handling.
>> 
>> My patch does (inadventently) change the behaviour of empty line
>> handling.  We should leave the current behaviour of skipping over empty
>> lines as is.  Suggested fix at the end of this message.
>> 
>> > Do you have a preference on how to proceed? We could add back a
>> > condition to the while loop, something like linebuf[n[0]]=='#' ||
>> > n[6]==len (i.e. skip lines with too few fields, possibly using a
>> > different number instead of 6 if more appropriate). Or we could do
>> > what I suggested before:
>> >
>> >> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
>> >> char* argument to collect the value before the " %d %d". Then you can
>> >> check for cnt<1. But I'm not sure even the 4th field should be
>> >> mandatory. This same apprach could be used to make just 3 mandatory if
>> >> desired though.
>> >
>> > Thoughts?
>> 
>> I think it would clearer to have an explicit check that the last
>> mandatory field is set, like I currently do with the mnt_fsname check at
>> the end of the function.  I don't particularly mind how many fields are
>> mandatory, as long as its four or fewer so Musl's behaviour follows the
>> fstab format described in the man page.
>> 
>> So overall my proposed revisions would be the following.  There's a
>> change to move to the next line if the current one is empty, and a
>> change to ensure the first four fields are all present.  (If you decide
>> you'd like the fourth field to also be optional, we can just change
>> mnt_opts to mnt_type in that check.)  It's been a long time since I last
>> this code, btw, so I hope I'm not missing anything around the empty line
>> check.  I'd be happy to put together a revised series, with these
>> changes and a corresponding change to my libc-test patch, if you'd like.
>> 
>> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
>> index 169e9789..7782cb10 100644
>> --- i/src/misc/mntent.c
>> +++ w/src/misc/mntent.c
>> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>>  			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
>>  			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
>>  			return 0;
>> -	} while (linebuf[n[0]] == '#');
>> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
>>  
>>  	linebuf[n[1]] = 0;
>>  	linebuf[n[3]] = 0;
>> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
>>  	mnt->mnt_type = linebuf+n[4];
>>  	mnt->mnt_opts = linebuf+n[6];
>>  
>> -	if (!*mnt->mnt_fsname)
>> +	if (!*mnt->mnt_opts)
>>  		return 0;
>>  
>>  	return mnt;
>
> What I still don't like here is that this changes the behavior on
> something that's not a valid record (in whatever sense we define that)
> from continuing the loop looking for the next one, to returning a null
> pointer with no indication of what the error was. Treating empty lines
> as an error (rather than continuing) was just one special case of
> that. For an analogy, see how the pwd/grp functions work. Something
> malformed in the file is just "not a record" and search continues for
> the next valid record (if any) rather than giving the caller a
> non-actionable (since this is assumed to be a sort of
> trusted/authoritative data outside the application's control) error.

Okay, that makes sense.  So it seems like our choices when faced with an
invalid record are:

 • Skip it and move on to the next one, as you've proposed here; or

 • Try to fill in as many fields of the mntent structure as possible,
   and return successfully, like Glibc does.

If we go with the first option, as you've proposed, do you think the
difference in behaviour from Glibc would be an issue?

(I'm mostly thinking of the case where a record doesn't have enough
fields here.  There are also the cases where a record has too many
fields, or when fields 5 and 6 are numeric.  I haven't looked into how
Glibc handles those yet.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-01-13 18:53           ` Alyssa Ross
@ 2022-05-12 14:08             ` Rich Felker
  2022-05-12 18:02               ` Alyssa Ross
                                 ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Rich Felker @ 2022-05-12 14:08 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: musl

On Thu, Jan 13, 2022 at 06:53:19PM +0000, Alyssa Ross wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> > On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
> >> Hi Rich, thanks for following up on this.
> >> 
> >> Rich Felker <dalias@libc.org> writes:
> >> 
> >> > On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> >> >> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> >> >> > According to fstab(5), the last two fields are optional, but this
> >> >> > wasn't accepted by Musl.  After this change, only the first field is
> >> >> > required, which matches Glibc's behaviour.
> >> >> > 
> >> >> > Using sscanf as before, it would have been impossible to differentiate
> >> >> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> >> >> > both cases due to the use of assignment suppression and %n for the
> >> >> > string fields (which is important to avoid copying any strings).  So
> >> >> > instead, before calling sscanf, initialize every string to the empty
> >> >> > string, and then we can check which strings are empty afterwards to
> >> >> > know how many fields were matched.
> >> >> > ---
> >> >> > 
> >> >> > We could also be stricter about it, and enforce that the first four
> >> >> > fields are present, since the man page says only the last two are
> >> >> > optional.  Doing that would be a simple change of checking for the
> >> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> >> >> > 
> >> >> > v2: don't change n from int to size_t
> >> >> > 
> >> >> >  src/misc/mntent.c | 18 +++++++++++++-----
> >> >> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >> >> > 
> >> >> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> >> >> > index eabb8200..238a0efd 100644
> >> >> > --- a/src/misc/mntent.c
> >> >> > +++ b/src/misc/mntent.c
> >> >> > @@ -21,7 +21,8 @@ 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);
> >> >> > +	int n[8], use_internal = (linebuf == SENTINEL);
> >> >> > +	size_t len, i;
> >> >> >  
> >> >> >  	mnt->mnt_freq = 0;
> >> >> >  	mnt->mnt_passno = 0;
> >> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >> >> >  			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]] == '#');
> >> >> > +
> >> >> > +		len = strlen(linebuf);
> >> >> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> >> >> > +		if (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) == EOF && ferror(f))
> >> >> > +			return 0;
> >> >> > +	} while (linebuf[n[0]] == '#');
> >> >> >  
> >> >> >  	linebuf[n[1]] = 0;
> >> >> >  	linebuf[n[3]] = 0;
> >> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >> >> >  	mnt->mnt_type = linebuf+n[4];
> >> >> >  	mnt->mnt_opts = linebuf+n[6];
> >> >> >  
> >> >> > +	if (!*mnt->mnt_fsname)
> >> >> > +		return 0;
> >> >> > +
> >> >> >  	return mnt;
> >> >> >  }
> >> >> 
> >> >> It looks like your patch changes the behavior for malformed lines from
> >> >> skipping them (and continuing to search for the next valid line) to
> >> >> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> >> >> won't it even cause blank lines to return 0?
> >> 
> >> Because I only check for the first field being present, a lot of
> >> nonsensical lines will be accepted.
> >> 
> >> As I said in the patch commentary:
> >> 
> >> > After this change, only the first field is
> >> > required, which matches Glibc's behaviour.
> >> >
> >> > [snip]
> >> >
> >> > We could also be stricter about it, and enforce that the first four
> >> > fields are present, since the man page says only the last two are
> >> > optional.  Doing that would be a simple change of checking for the
> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> >> 
> >> It probably would make more sense to check that the four fields the man
> >> pages implies are required are all there, by making the change I
> >> suggested in the commentary, at least until somebody complains about
> >> their two-field fstab being accepted by Glibc and not Musl.
> >> 
> >> > Indeed it also seems to be skipping empty lines, contrary to what you
> >> > said in another message:
> >> >
> >> >>  • Empty lines should be skipped.
> >> 
> >> Yes, it looks like I was mistaken before when I thought that Musl didn't
> >> properly handle comments and empty lines.  Looking back, it seems that
> >> the tests I was running were against mntent files with only four fields,
> >> so the parsing failures I was seeing were because of that, not because
> >> of an issue in the comment or empty line handling.
> >> 
> >> My patch does (inadventently) change the behaviour of empty line
> >> handling.  We should leave the current behaviour of skipping over empty
> >> lines as is.  Suggested fix at the end of this message.
> >> 
> >> > Do you have a preference on how to proceed? We could add back a
> >> > condition to the while loop, something like linebuf[n[0]]=='#' ||
> >> > n[6]==len (i.e. skip lines with too few fields, possibly using a
> >> > different number instead of 6 if more appropriate). Or we could do
> >> > what I suggested before:
> >> >
> >> >> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> >> >> char* argument to collect the value before the " %d %d". Then you can
> >> >> check for cnt<1. But I'm not sure even the 4th field should be
> >> >> mandatory. This same apprach could be used to make just 3 mandatory if
> >> >> desired though.
> >> >
> >> > Thoughts?
> >> 
> >> I think it would clearer to have an explicit check that the last
> >> mandatory field is set, like I currently do with the mnt_fsname check at
> >> the end of the function.  I don't particularly mind how many fields are
> >> mandatory, as long as its four or fewer so Musl's behaviour follows the
> >> fstab format described in the man page.
> >> 
> >> So overall my proposed revisions would be the following.  There's a
> >> change to move to the next line if the current one is empty, and a
> >> change to ensure the first four fields are all present.  (If you decide
> >> you'd like the fourth field to also be optional, we can just change
> >> mnt_opts to mnt_type in that check.)  It's been a long time since I last
> >> this code, btw, so I hope I'm not missing anything around the empty line
> >> check.  I'd be happy to put together a revised series, with these
> >> changes and a corresponding change to my libc-test patch, if you'd like.
> >> 
> >> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
> >> index 169e9789..7782cb10 100644
> >> --- i/src/misc/mntent.c
> >> +++ w/src/misc/mntent.c
> >> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >>  			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> >>  			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
> >>  			return 0;
> >> -	} while (linebuf[n[0]] == '#');
> >> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
> >>  
> >>  	linebuf[n[1]] = 0;
> >>  	linebuf[n[3]] = 0;
> >> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >>  	mnt->mnt_type = linebuf+n[4];
> >>  	mnt->mnt_opts = linebuf+n[6];
> >>  
> >> -	if (!*mnt->mnt_fsname)
> >> +	if (!*mnt->mnt_opts)
> >>  		return 0;
> >>  
> >>  	return mnt;
> >
> > What I still don't like here is that this changes the behavior on
> > something that's not a valid record (in whatever sense we define that)
> > from continuing the loop looking for the next one, to returning a null
> > pointer with no indication of what the error was. Treating empty lines
> > as an error (rather than continuing) was just one special case of
> > that. For an analogy, see how the pwd/grp functions work. Something
> > malformed in the file is just "not a record" and search continues for
> > the next valid record (if any) rather than giving the caller a
> > non-actionable (since this is assumed to be a sort of
> > trusted/authoritative data outside the application's control) error.

Just getting back to this now after the release.. Apologies for the
long delay.

> Okay, that makes sense.  So it seems like our choices when faced with an
> invalid record are:
> 
>  • Skip it and move on to the next one, as you've proposed here; or
> 
>  • Try to fill in as many fields of the mntent structure as possible,
>    and return successfully, like Glibc does.
> 
> If we go with the first option, as you've proposed, do you think the
> difference in behaviour from Glibc would be an issue?

I think the latter is better. If I remember correctly what we'd found,
glibc hardly has any condition on what's mandatory (just first field?
or first and second?) so I think the condition would be something
like:

		...
	} while (linebuf[n[0]] == '#' || n[1]==len);

or similar, possibly replacing 1 with 3 if 2 fields are mandatory,
etc.

> (I'm mostly thinking of the case where a record doesn't have enough
> fields here.  There are also the cases where a record has too many
> fields, or when fields 5 and 6 are numeric.  I haven't looked into how
> Glibc handles those yet.)

A few other changes I think should be made:

- The two numeric fields should be read into temporaries initialized
  with default values so that it doesn't matter if they're read or
  not.

- The n[] array should be changed to size_t[] and the %n's to %zn's.
  This should actually be done as a separate change, as it's a fix for
  a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
  the buffer length potentially longer than INT_MAX.

- There's also an independent bug in hasmntent that was reported a
  long time ago then lost: it will return false positives when one
  mntopt name is a substring of another. strstr is just not the right
  operation here, at least not without added logic to ensure matching
  on a whole option boundary. This is a separate issue that calls for
  a separate patch though, not a blocker on the patch under discussion
  here.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 14:08             ` Rich Felker
@ 2022-05-12 18:02               ` Alyssa Ross
  2022-05-12 18:15                 ` Rich Felker
  2022-05-12 20:58               ` Oliver Ford
  2022-05-15 23:31               ` Rich Felker
  2 siblings, 1 reply; 22+ messages in thread
From: Alyssa Ross @ 2022-05-12 18:02 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 11598 bytes --]

On Thu, May 12, 2022 at 10:08:37AM -0400, Rich Felker wrote:
> On Thu, Jan 13, 2022 at 06:53:19PM +0000, Alyssa Ross wrote:
> > Rich Felker <dalias@libc.org> writes:
> >
> > > On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
> > >> Hi Rich, thanks for following up on this.
> > >>
> > >> Rich Felker <dalias@libc.org> writes:
> > >>
> > >> > On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> > >> >> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> > >> >> > According to fstab(5), the last two fields are optional, but this
> > >> >> > wasn't accepted by Musl.  After this change, only the first field is
> > >> >> > required, which matches Glibc's behaviour.
> > >> >> >
> > >> >> > Using sscanf as before, it would have been impossible to differentiate
> > >> >> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> > >> >> > both cases due to the use of assignment suppression and %n for the
> > >> >> > string fields (which is important to avoid copying any strings).  So
> > >> >> > instead, before calling sscanf, initialize every string to the empty
> > >> >> > string, and then we can check which strings are empty afterwards to
> > >> >> > know how many fields were matched.
> > >> >> > ---
> > >> >> >
> > >> >> > We could also be stricter about it, and enforce that the first four
> > >> >> > fields are present, since the man page says only the last two are
> > >> >> > optional.  Doing that would be a simple change of checking for the
> > >> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > >> >> >
> > >> >> > v2: don't change n from int to size_t
> > >> >> >
> > >> >> >  src/misc/mntent.c | 18 +++++++++++++-----
> > >> >> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > >> >> >
> > >> >> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> > >> >> > index eabb8200..238a0efd 100644
> > >> >> > --- a/src/misc/mntent.c
> > >> >> > +++ b/src/misc/mntent.c
> > >> >> > @@ -21,7 +21,8 @@ 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);
> > >> >> > +	int n[8], use_internal = (linebuf == SENTINEL);
> > >> >> > +	size_t len, i;
> > >> >> >
> > >> >> >  	mnt->mnt_freq = 0;
> > >> >> >  	mnt->mnt_passno = 0;
> > >> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > >> >> >  			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]] == '#');
> > >> >> > +
> > >> >> > +		len = strlen(linebuf);
> > >> >> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> > >> >> > +		if (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) == EOF && ferror(f))
> > >> >> > +			return 0;
> > >> >> > +	} while (linebuf[n[0]] == '#');
> > >> >> >
> > >> >> >  	linebuf[n[1]] = 0;
> > >> >> >  	linebuf[n[3]] = 0;
> > >> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > >> >> >  	mnt->mnt_type = linebuf+n[4];
> > >> >> >  	mnt->mnt_opts = linebuf+n[6];
> > >> >> >
> > >> >> > +	if (!*mnt->mnt_fsname)
> > >> >> > +		return 0;
> > >> >> > +
> > >> >> >  	return mnt;
> > >> >> >  }
> > >> >>
> > >> >> It looks like your patch changes the behavior for malformed lines from
> > >> >> skipping them (and continuing to search for the next valid line) to
> > >> >> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> > >> >> won't it even cause blank lines to return 0?
> > >>
> > >> Because I only check for the first field being present, a lot of
> > >> nonsensical lines will be accepted.
> > >>
> > >> As I said in the patch commentary:
> > >>
> > >> > After this change, only the first field is
> > >> > required, which matches Glibc's behaviour.
> > >> >
> > >> > [snip]
> > >> >
> > >> > We could also be stricter about it, and enforce that the first four
> > >> > fields are present, since the man page says only the last two are
> > >> > optional.  Doing that would be a simple change of checking for the
> > >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > >>
> > >> It probably would make more sense to check that the four fields the man
> > >> pages implies are required are all there, by making the change I
> > >> suggested in the commentary, at least until somebody complains about
> > >> their two-field fstab being accepted by Glibc and not Musl.
> > >>
> > >> > Indeed it also seems to be skipping empty lines, contrary to what you
> > >> > said in another message:
> > >> >
> > >> >>  • Empty lines should be skipped.
> > >>
> > >> Yes, it looks like I was mistaken before when I thought that Musl didn't
> > >> properly handle comments and empty lines.  Looking back, it seems that
> > >> the tests I was running were against mntent files with only four fields,
> > >> so the parsing failures I was seeing were because of that, not because
> > >> of an issue in the comment or empty line handling.
> > >>
> > >> My patch does (inadventently) change the behaviour of empty line
> > >> handling.  We should leave the current behaviour of skipping over empty
> > >> lines as is.  Suggested fix at the end of this message.
> > >>
> > >> > Do you have a preference on how to proceed? We could add back a
> > >> > condition to the while loop, something like linebuf[n[0]]=='#' ||
> > >> > n[6]==len (i.e. skip lines with too few fields, possibly using a
> > >> > different number instead of 6 if more appropriate). Or we could do
> > >> > what I suggested before:
> > >> >
> > >> >> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> > >> >> char* argument to collect the value before the " %d %d". Then you can
> > >> >> check for cnt<1. But I'm not sure even the 4th field should be
> > >> >> mandatory. This same apprach could be used to make just 3 mandatory if
> > >> >> desired though.
> > >> >
> > >> > Thoughts?
> > >>
> > >> I think it would clearer to have an explicit check that the last
> > >> mandatory field is set, like I currently do with the mnt_fsname check at
> > >> the end of the function.  I don't particularly mind how many fields are
> > >> mandatory, as long as its four or fewer so Musl's behaviour follows the
> > >> fstab format described in the man page.
> > >>
> > >> So overall my proposed revisions would be the following.  There's a
> > >> change to move to the next line if the current one is empty, and a
> > >> change to ensure the first four fields are all present.  (If you decide
> > >> you'd like the fourth field to also be optional, we can just change
> > >> mnt_opts to mnt_type in that check.)  It's been a long time since I last
> > >> this code, btw, so I hope I'm not missing anything around the empty line
> > >> check.  I'd be happy to put together a revised series, with these
> > >> changes and a corresponding change to my libc-test patch, if you'd like.
> > >>
> > >> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
> > >> index 169e9789..7782cb10 100644
> > >> --- i/src/misc/mntent.c
> > >> +++ w/src/misc/mntent.c
> > >> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > >>  			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> > >>  			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
> > >>  			return 0;
> > >> -	} while (linebuf[n[0]] == '#');
> > >> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
> > >>
> > >>  	linebuf[n[1]] = 0;
> > >>  	linebuf[n[3]] = 0;
> > >> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > >>  	mnt->mnt_type = linebuf+n[4];
> > >>  	mnt->mnt_opts = linebuf+n[6];
> > >>
> > >> -	if (!*mnt->mnt_fsname)
> > >> +	if (!*mnt->mnt_opts)
> > >>  		return 0;
> > >>
> > >>  	return mnt;
> > >
> > > What I still don't like here is that this changes the behavior on
> > > something that's not a valid record (in whatever sense we define that)
> > > from continuing the loop looking for the next one, to returning a null
> > > pointer with no indication of what the error was. Treating empty lines
> > > as an error (rather than continuing) was just one special case of
> > > that. For an analogy, see how the pwd/grp functions work. Something
> > > malformed in the file is just "not a record" and search continues for
> > > the next valid record (if any) rather than giving the caller a
> > > non-actionable (since this is assumed to be a sort of
> > > trusted/authoritative data outside the application's control) error.
>
> Just getting back to this now after the release.. Apologies for the
> long delay.
>
> > Okay, that makes sense.  So it seems like our choices when faced with an
> > invalid record are:
> >
> >  • Skip it and move on to the next one, as you've proposed here; or
> >
> >  • Try to fill in as many fields of the mntent structure as possible,
> >    and return successfully, like Glibc does.
> >
> > If we go with the first option, as you've proposed, do you think the
> > difference in behaviour from Glibc would be an issue?
>
> I think the latter is better. If I remember correctly what we'd found,
> glibc hardly has any condition on what's mandatory (just first field?
> or first and second?) so I think the condition would be something
> like:
>
> 		...
> 	} while (linebuf[n[0]] == '#' || n[1]==len);
>
> or similar, possibly replacing 1 with 3 if 2 fields are mandatory,
> etc.

I'm pretty sure it was just the first field that's mandatory with Glibc.

> > (I'm mostly thinking of the case where a record doesn't have enough
> > fields here.  There are also the cases where a record has too many
> > fields, or when fields 5 and 6 are numeric.  I haven't looked into how
> > Glibc handles those yet.)
>
> A few other changes I think should be made:
>
> - The two numeric fields should be read into temporaries initialized
>   with default values so that it doesn't matter if they're read or
>   not.

Does that make a difference?  We already explicitly zero them at the
start of the function.  (Maybe I'm forgetting some scanf arcana — it's
been a while since I've thought about this.)

> - The n[] array should be changed to size_t[] and the %n's to %zn's.
>   This should actually be done as a separate change, as it's a fix for
>   a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
>   the buffer length potentially longer than INT_MAX.

Yes.  IIRC I half did this in my original attempt at this patch, but
didn't change the format specifiers.

> - There's also an independent bug in hasmntent that was reported a
>   long time ago then lost: it will return false positives when one
>   mntopt name is a substring of another. strstr is just not the right
>   operation here, at least not without added logic to ensure matching
>   on a whole option boundary. This is a separate issue that calls for
>   a separate patch though, not a blocker on the patch under discussion
>   here.
>
> Rich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 18:02               ` Alyssa Ross
@ 2022-05-12 18:15                 ` Rich Felker
  2022-05-15 18:38                   ` Alyssa Ross
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-05-12 18:15 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: musl

On Thu, May 12, 2022 at 06:02:35PM +0000, Alyssa Ross wrote:
> On Thu, May 12, 2022 at 10:08:37AM -0400, Rich Felker wrote:
> > On Thu, Jan 13, 2022 at 06:53:19PM +0000, Alyssa Ross wrote:
> > > Rich Felker <dalias@libc.org> writes:
> > >
> > > > On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
> > > >> Hi Rich, thanks for following up on this.
> > > >>
> > > >> Rich Felker <dalias@libc.org> writes:
> > > >>
> > > >> > On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> > > >> >> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> > > >> >> > According to fstab(5), the last two fields are optional, but this
> > > >> >> > wasn't accepted by Musl.  After this change, only the first field is
> > > >> >> > required, which matches Glibc's behaviour.
> > > >> >> >
> > > >> >> > Using sscanf as before, it would have been impossible to differentiate
> > > >> >> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> > > >> >> > both cases due to the use of assignment suppression and %n for the
> > > >> >> > string fields (which is important to avoid copying any strings).  So
> > > >> >> > instead, before calling sscanf, initialize every string to the empty
> > > >> >> > string, and then we can check which strings are empty afterwards to
> > > >> >> > know how many fields were matched.
> > > >> >> > ---
> > > >> >> >
> > > >> >> > We could also be stricter about it, and enforce that the first four
> > > >> >> > fields are present, since the man page says only the last two are
> > > >> >> > optional.  Doing that would be a simple change of checking for the
> > > >> >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > > >> >> >
> > > >> >> > v2: don't change n from int to size_t
> > > >> >> >
> > > >> >> >  src/misc/mntent.c | 18 +++++++++++++-----
> > > >> >> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > >> >> >
> > > >> >> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> > > >> >> > index eabb8200..238a0efd 100644
> > > >> >> > --- a/src/misc/mntent.c
> > > >> >> > +++ b/src/misc/mntent.c
> > > >> >> > @@ -21,7 +21,8 @@ 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);
> > > >> >> > +	int n[8], use_internal = (linebuf == SENTINEL);
> > > >> >> > +	size_t len, i;
> > > >> >> >
> > > >> >> >  	mnt->mnt_freq = 0;
> > > >> >> >  	mnt->mnt_passno = 0;
> > > >> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > > >> >> >  			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]] == '#');
> > > >> >> > +
> > > >> >> > +		len = strlen(linebuf);
> > > >> >> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> > > >> >> > +		if (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) == EOF && ferror(f))
> > > >> >> > +			return 0;
> > > >> >> > +	} while (linebuf[n[0]] == '#');
> > > >> >> >
> > > >> >> >  	linebuf[n[1]] = 0;
> > > >> >> >  	linebuf[n[3]] = 0;
> > > >> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > > >> >> >  	mnt->mnt_type = linebuf+n[4];
> > > >> >> >  	mnt->mnt_opts = linebuf+n[6];
> > > >> >> >
> > > >> >> > +	if (!*mnt->mnt_fsname)
> > > >> >> > +		return 0;
> > > >> >> > +
> > > >> >> >  	return mnt;
> > > >> >> >  }
> > > >> >>
> > > >> >> It looks like your patch changes the behavior for malformed lines from
> > > >> >> skipping them (and continuing to search for the next valid line) to
> > > >> >> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> > > >> >> won't it even cause blank lines to return 0?
> > > >>
> > > >> Because I only check for the first field being present, a lot of
> > > >> nonsensical lines will be accepted.
> > > >>
> > > >> As I said in the patch commentary:
> > > >>
> > > >> > After this change, only the first field is
> > > >> > required, which matches Glibc's behaviour.
> > > >> >
> > > >> > [snip]
> > > >> >
> > > >> > We could also be stricter about it, and enforce that the first four
> > > >> > fields are present, since the man page says only the last two are
> > > >> > optional.  Doing that would be a simple change of checking for the
> > > >> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > > >>
> > > >> It probably would make more sense to check that the four fields the man
> > > >> pages implies are required are all there, by making the change I
> > > >> suggested in the commentary, at least until somebody complains about
> > > >> their two-field fstab being accepted by Glibc and not Musl.
> > > >>
> > > >> > Indeed it also seems to be skipping empty lines, contrary to what you
> > > >> > said in another message:
> > > >> >
> > > >> >>  • Empty lines should be skipped.
> > > >>
> > > >> Yes, it looks like I was mistaken before when I thought that Musl didn't
> > > >> properly handle comments and empty lines.  Looking back, it seems that
> > > >> the tests I was running were against mntent files with only four fields,
> > > >> so the parsing failures I was seeing were because of that, not because
> > > >> of an issue in the comment or empty line handling.
> > > >>
> > > >> My patch does (inadventently) change the behaviour of empty line
> > > >> handling.  We should leave the current behaviour of skipping over empty
> > > >> lines as is.  Suggested fix at the end of this message.
> > > >>
> > > >> > Do you have a preference on how to proceed? We could add back a
> > > >> > condition to the while loop, something like linebuf[n[0]]=='#' ||
> > > >> > n[6]==len (i.e. skip lines with too few fields, possibly using a
> > > >> > different number instead of 6 if more appropriate). Or we could do
> > > >> > what I suggested before:
> > > >> >
> > > >> >> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> > > >> >> char* argument to collect the value before the " %d %d". Then you can
> > > >> >> check for cnt<1. But I'm not sure even the 4th field should be
> > > >> >> mandatory. This same apprach could be used to make just 3 mandatory if
> > > >> >> desired though.
> > > >> >
> > > >> > Thoughts?
> > > >>
> > > >> I think it would clearer to have an explicit check that the last
> > > >> mandatory field is set, like I currently do with the mnt_fsname check at
> > > >> the end of the function.  I don't particularly mind how many fields are
> > > >> mandatory, as long as its four or fewer so Musl's behaviour follows the
> > > >> fstab format described in the man page.
> > > >>
> > > >> So overall my proposed revisions would be the following.  There's a
> > > >> change to move to the next line if the current one is empty, and a
> > > >> change to ensure the first four fields are all present.  (If you decide
> > > >> you'd like the fourth field to also be optional, we can just change
> > > >> mnt_opts to mnt_type in that check.)  It's been a long time since I last
> > > >> this code, btw, so I hope I'm not missing anything around the empty line
> > > >> check.  I'd be happy to put together a revised series, with these
> > > >> changes and a corresponding change to my libc-test patch, if you'd like.
> > > >>
> > > >> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
> > > >> index 169e9789..7782cb10 100644
> > > >> --- i/src/misc/mntent.c
> > > >> +++ w/src/misc/mntent.c
> > > >> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > > >>  			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> > > >>  			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
> > > >>  			return 0;
> > > >> -	} while (linebuf[n[0]] == '#');
> > > >> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
> > > >>
> > > >>  	linebuf[n[1]] = 0;
> > > >>  	linebuf[n[3]] = 0;
> > > >> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > > >>  	mnt->mnt_type = linebuf+n[4];
> > > >>  	mnt->mnt_opts = linebuf+n[6];
> > > >>
> > > >> -	if (!*mnt->mnt_fsname)
> > > >> +	if (!*mnt->mnt_opts)
> > > >>  		return 0;
> > > >>
> > > >>  	return mnt;
> > > >
> > > > What I still don't like here is that this changes the behavior on
> > > > something that's not a valid record (in whatever sense we define that)
> > > > from continuing the loop looking for the next one, to returning a null
> > > > pointer with no indication of what the error was. Treating empty lines
> > > > as an error (rather than continuing) was just one special case of
> > > > that. For an analogy, see how the pwd/grp functions work. Something
> > > > malformed in the file is just "not a record" and search continues for
> > > > the next valid record (if any) rather than giving the caller a
> > > > non-actionable (since this is assumed to be a sort of
> > > > trusted/authoritative data outside the application's control) error.
> >
> > Just getting back to this now after the release.. Apologies for the
> > long delay.
> >
> > > Okay, that makes sense.  So it seems like our choices when faced with an
> > > invalid record are:
> > >
> > >  • Skip it and move on to the next one, as you've proposed here; or
> > >
> > >  • Try to fill in as many fields of the mntent structure as possible,
> > >    and return successfully, like Glibc does.
> > >
> > > If we go with the first option, as you've proposed, do you think the
> > > difference in behaviour from Glibc would be an issue?
> >
> > I think the latter is better. If I remember correctly what we'd found,
> > glibc hardly has any condition on what's mandatory (just first field?
> > or first and second?) so I think the condition would be something
> > like:
> >
> > 		...
> > 	} while (linebuf[n[0]] == '#' || n[1]==len);
> >
> > or similar, possibly replacing 1 with 3 if 2 fields are mandatory,
> > etc.
> 
> I'm pretty sure it was just the first field that's mandatory with Glibc.

OK.

> > > (I'm mostly thinking of the case where a record doesn't have enough
> > > fields here.  There are also the cases where a record has too many
> > > fields, or when fields 5 and 6 are numeric.  I haven't looked into how
> > > Glibc handles those yet.)
> >
> > A few other changes I think should be made:
> >
> > - The two numeric fields should be read into temporaries initialized
> >   with default values so that it doesn't matter if they're read or
> >   not.
> 
> Does that make a difference?  We already explicitly zero them at the
> start of the function.  (Maybe I'm forgetting some scanf arcana — it's
> been a while since I've thought about this.)

Oh, I missed that. In that case I think it's okay as-is. As general
policy, I'd rather not modify the caller's object until we're
committed to storing a successful result in it, but changing that is
orthogonal to your patch and can be done separately if wanted.

> > - The n[] array should be changed to size_t[] and the %n's to %zn's.
> >   This should actually be done as a separate change, as it's a fix for
> >   a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
> >   the buffer length potentially longer than INT_MAX.
> 
> Yes.  IIRC I half did this in my original attempt at this patch, but
> didn't change the format specifiers.

Alright. It sounds like your patch is pretty much okay as-is except
for the added " || n[1]==len" in the while condition and removing the
(now unreachable) return-zero condition at the end. If that sounds
right and it's okay with you I'm happy to commit your existing patch
with those changes then make any other improvements myself or review
further patches from you separately if you like. What do you think?

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 14:08             ` Rich Felker
  2022-05-12 18:02               ` Alyssa Ross
@ 2022-05-12 20:58               ` Oliver Ford
  2022-05-12 21:10                 ` Rich Felker
  2022-05-15 23:31               ` Rich Felker
  2 siblings, 1 reply; 22+ messages in thread
From: Oliver Ford @ 2022-05-12 20:58 UTC (permalink / raw)
  To: musl

On Thu, May 12, 2022 at 3:08 PM Rich Felker <dalias@libc.org> wrote:
>
> - There's also an independent bug in hasmntent that was reported a
>   long time ago then lost: it will return false positives when one
>   mntopt name is a substring of another. strstr is just not the right
>   operation here, at least not without added logic to ensure matching
>   on a whole option boundary. This is a separate issue that calls for
>   a separate patch though, not a blocker on the patch under discussion
>   here.
>
Looking at this, the "hasmntopt" function does match options where the
string is part of the option but not the whole option. So the opt "ro"
will correctly match the "ro" (for read-only) option, but also match
an option that contains "symlinkroot".

The following version of the function keeps the initial strstr, but
adds an extra check so that it doesn't match unless the next character
is either a comma or nul. If there's no other special cases we need to
handle, I'll submit as a
patch?

char *hasmntopt(const struct mntent *mnt, const char *opt)
{
        char *op = strstr(mnt->mnt_opts, opt);

        if (op == NULL) return NULL;
        size_t len = strlen(opt);
        char *end = op + len;
        if (*end == '\0' || *end == ',') return op;

        return NULL;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 20:58               ` Oliver Ford
@ 2022-05-12 21:10                 ` Rich Felker
  2022-05-13 21:39                   ` Oliver Ford
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-05-12 21:10 UTC (permalink / raw)
  To: Oliver Ford; +Cc: musl

On Thu, May 12, 2022 at 09:58:30PM +0100, Oliver Ford wrote:
> On Thu, May 12, 2022 at 3:08 PM Rich Felker <dalias@libc.org> wrote:
> >
> > - There's also an independent bug in hasmntent that was reported a
> >   long time ago then lost: it will return false positives when one
> >   mntopt name is a substring of another. strstr is just not the right
> >   operation here, at least not without added logic to ensure matching
> >   on a whole option boundary. This is a separate issue that calls for
> >   a separate patch though, not a blocker on the patch under discussion
> >   here.
> >
> Looking at this, the "hasmntopt" function does match options where the
> string is part of the option but not the whole option. So the opt "ro"
> will correctly match the "ro" (for read-only) option, but also match
> an option that contains "symlinkroot".
> 
> The following version of the function keeps the initial strstr, but
> adds an extra check so that it doesn't match unless the next character
> is either a comma or nul. If there's no other special cases we need to
> handle, I'll submit as a
> patch?
> 
> char *hasmntopt(const struct mntent *mnt, const char *opt)
> {
>         char *op = strstr(mnt->mnt_opts, opt);
> 
>         if (op == NULL) return NULL;
>         size_t len = strlen(opt);
>         char *end = op + len;
>         if (*end == '\0' || *end == ',') return op;
> 
>         return NULL;
> }

This fails to check that the match is at the start of an option
(preceded by a ',' or at the beginning of string) and fails to
continue if the first match is a false positive (e.g. "ro" in
"symlinkroot,ro"). It's possible to solve this still using strstr in a
loop, but it might be easier to just iterate delimiters and strcmp.

Also, I'm not sure if hasmntopt is supposed to return a match or not
for something like "uid" in "uid=1001"; if so, being followed by '='
also needs to be considered valid match.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 21:10                 ` Rich Felker
@ 2022-05-13 21:39                   ` Oliver Ford
  2022-05-14  4:24                     ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Ford @ 2022-05-13 21:39 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

On Thu, May 12, 2022 at 10:10 PM Rich Felker <dalias@libc.org> wrote:
> This fails to check that the match is at the start of an option
> (preceded by a ',' or at the beginning of string) and fails to
> continue if the first match is a false positive (e.g. "ro" in
> "symlinkroot,ro"). It's possible to solve this still using strstr in a
> loop, but it might be easier to just iterate delimiters and strcmp.
>
> Also, I'm not sure if hasmntopt is supposed to return a match or not
> for something like "uid" in "uid=1001"; if so, being followed by '='
> also needs to be considered valid match.
>
> Rich

Comparing glibc and bionic, they both match when followed by an '='.
So the below function handles that, and replaces strstr with an
strncmp and a loop. If this version is ok I'll submit a patch?

char *hasmntopt(const struct mntent *mnt, const char *opt)
{
    char *ptr = mnt->mnt_opts;
    size_t len = strlen(opt);

    while (ptr) {
        char *end = ptr + len;
        if (!strncmp(ptr, opt, len) &&
                 (*end == '\0' || *end == ',' || *end == '=')) return ptr;
        ptr = strchr(ptr, ',');
        if (ptr) ptr++;
    }

    return NULL;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-13 21:39                   ` Oliver Ford
@ 2022-05-14  4:24                     ` Rich Felker
  2022-05-14 22:16                       ` Oliver Ford
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-05-14  4:24 UTC (permalink / raw)
  To: Oliver Ford; +Cc: musl

On Fri, May 13, 2022 at 10:39:17PM +0100, Oliver Ford wrote:
> On Thu, May 12, 2022 at 10:10 PM Rich Felker <dalias@libc.org> wrote:
> > This fails to check that the match is at the start of an option
> > (preceded by a ',' or at the beginning of string) and fails to
> > continue if the first match is a false positive (e.g. "ro" in
> > "symlinkroot,ro"). It's possible to solve this still using strstr in a
> > loop, but it might be easier to just iterate delimiters and strcmp.
> >
> > Also, I'm not sure if hasmntopt is supposed to return a match or not
> > for something like "uid" in "uid=1001"; if so, being followed by '='
> > also needs to be considered valid match.
> >
> > Rich
> 
> Comparing glibc and bionic, they both match when followed by an '='.
> So the below function handles that, and replaces strstr with an
> strncmp and a loop. If this version is ok I'll submit a patch?
> 
> char *hasmntopt(const struct mntent *mnt, const char *opt)
> {
>     char *ptr = mnt->mnt_opts;
>     size_t len = strlen(opt);
> 
>     while (ptr) {
>         char *end = ptr + len;

This has UB unless ptr points to at least len bytes. You can only
evaluate ptr+len or ptr[len] after confirming that. The strncmp is the
most natural (and free) way of confirming it. You could assign end
after the strncmp but I think just using ptr[len] instead of *end
makes it easier.

>         if (!strncmp(ptr, opt, len) &&
>                  (*end == '\0' || *end == ',' || *end == '=')) return ptr;
>         ptr = strchr(ptr, ',');
>         if (ptr) ptr++;
>     }
> 
>     return NULL;

Minor style nit: NULL is generally not used in musl anymore; just 0 is
fine here.

BTW you dropped the list from CC; I re-added it.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-14  4:24                     ` Rich Felker
@ 2022-05-14 22:16                       ` Oliver Ford
  0 siblings, 0 replies; 22+ messages in thread
From: Oliver Ford @ 2022-05-14 22:16 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

On Sat, May 14, 2022 at 5:24 AM Rich Felker <dalias@libc.org> wrote:
> This has UB unless ptr points to at least len bytes. You can only
> evaluate ptr+len or ptr[len] after confirming that. The strncmp is the
> most natural (and free) way of confirming it. You could assign end
> after the strncmp but I think just using ptr[len] instead of *end
> makes it easier.
>
> >         if (!strncmp(ptr, opt, len) &&
> >                  (*end == '\0' || *end == ',' || *end == '=')) return ptr;
> >         ptr = strchr(ptr, ',');
> >         if (ptr) ptr++;
> >     }
> >
> >     return NULL;
>
> Minor style nit: NULL is generally not used in musl anymore; just 0 is
> fine here.
>
> BTW you dropped the list from CC; I re-added it.
>
> Rich

Version below should be correct, now uses ptr[len] instead of the *end.

char *hasmntopt(const struct mntent *mnt, const char *opt)
{
    char *ptr = mnt->mnt_opts;
    size_t len = strlen(opt);

    while (ptr) {
        if (!strncmp(ptr, opt, len) &&
            (ptr[len] == '\0' || ptr[len] == ',' || ptr[len] == '='))
return ptr;
        ptr = strchr(ptr, ',');
        if (ptr) ptr++;
    }

    return 0;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 18:15                 ` Rich Felker
@ 2022-05-15 18:38                   ` Alyssa Ross
  2022-05-15 23:19                     ` Rich Felker
  0 siblings, 1 reply; 22+ messages in thread
From: Alyssa Ross @ 2022-05-15 18:38 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

Hi Rich,

On Thu, May 12, 2022 at 02:15:53PM -0400, Rich Felker wrote:
> Alright. It sounds like your patch is pretty much okay as-is except
> for the added " || n[1]==len" in the while condition and removing the
> (now unreachable) return-zero condition at the end. If that sounds
> right and it's okay with you I'm happy to commit your existing patch
> with those changes then make any other improvements myself or review
> further patches from you separately if you like. What do you think?

Sounds good.  Please go ahead and do it yourself —-it would take me a
fair bit of time to put my Musl development setup back together, so if
you're happy to fix up and commit the patch there's no point in me
doing that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-15 18:38                   ` Alyssa Ross
@ 2022-05-15 23:19                     ` Rich Felker
  2022-05-16 10:19                       ` Alyssa Ross
  0 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-05-15 23:19 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: musl

On Sun, May 15, 2022 at 06:38:40PM +0000, Alyssa Ross wrote:
> Hi Rich,
> 
> On Thu, May 12, 2022 at 02:15:53PM -0400, Rich Felker wrote:
> > Alright. It sounds like your patch is pretty much okay as-is except
> > for the added " || n[1]==len" in the while condition and removing the
> > (now unreachable) return-zero condition at the end. If that sounds
> > right and it's okay with you I'm happy to commit your existing patch
> > with those changes then make any other improvements myself or review
> > further patches from you separately if you like. What do you think?
> 
> Sounds good.  Please go ahead and do it yourself —-it would take me a
> fair bit of time to put my Musl development setup back together, so if
> you're happy to fix up and commit the patch there's no point in me
> doing that.

One more thing I missed: checking result of sscanf for EOF and
ferror(f) does not make sense; that would only make sense if it were
fscanf, but the FILE access already happened and was checked earlier.
Any objection to me just also removing that from the patch?

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-12 14:08             ` Rich Felker
  2022-05-12 18:02               ` Alyssa Ross
  2022-05-12 20:58               ` Oliver Ford
@ 2022-05-15 23:31               ` Rich Felker
  2022-05-16 10:07                 ` Alyssa Ross
  2 siblings, 1 reply; 22+ messages in thread
From: Rich Felker @ 2022-05-15 23:31 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: musl

On Thu, May 12, 2022 at 10:08:37AM -0400, Rich Felker wrote:
> A few other changes I think should be made:
> 
> [...]
> 
> - The n[] array should be changed to size_t[] and the %n's to %zn's.
>   This should actually be done as a separate change, as it's a fix for
>   a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
>   the buffer length potentially longer than INT_MAX.

I'm going to reverse position on this: since getmntent_r only accepts
an int for the line length, the thread-unsafe getmntent should not be
able to process longer lines either. I'll instead just use the len you
already computed to treat lines longer than INT_MAX as invalid.

Rich

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-15 23:31               ` Rich Felker
@ 2022-05-16 10:07                 ` Alyssa Ross
  0 siblings, 0 replies; 22+ messages in thread
From: Alyssa Ross @ 2022-05-16 10:07 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Sun, May 15, 2022 at 07:31:55PM -0400, Rich Felker wrote:
> On Thu, May 12, 2022 at 10:08:37AM -0400, Rich Felker wrote:
> > A few other changes I think should be made:
> >
> > [...]
> >
> > - The n[] array should be changed to size_t[] and the %n's to %zn's.
> >   This should actually be done as a separate change, as it's a fix for
> >   a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
> >   the buffer length potentially longer than INT_MAX.
>
> I'm going to reverse position on this: since getmntent_r only accepts
> an int for the line length, the thread-unsafe getmntent should not be
> able to process longer lines either. I'll instead just use the len you
> already computed to treat lines longer than INT_MAX as invalid.

Makes sense.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields
  2022-05-15 23:19                     ` Rich Felker
@ 2022-05-16 10:19                       ` Alyssa Ross
  0 siblings, 0 replies; 22+ messages in thread
From: Alyssa Ross @ 2022-05-16 10:19 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

On Sun, May 15, 2022 at 07:19:29PM -0400, Rich Felker wrote:
> One more thing I missed: checking result of sscanf for EOF and
> ferror(f) does not make sense; that would only make sense if it were
> fscanf, but the FILE access already happened and was checked earlier.
> Any objection to me just also removing that from the patch?

That sounds fine to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-05-16 10:19 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 22:11 [musl] [PATCH v2 0/3] Alyssa Ross
2021-09-15 22:11 ` [musl] [PATCH libc-test v2 1/3] functional: add mntent test Alyssa Ross
2021-09-15 22:11 ` [musl] [PATCH libc-test v2 2/3] functional: add mntent test for single-field line Alyssa Ross
2021-09-15 22:11 ` [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields Alyssa Ross
2021-09-20  4:21   ` Rich Felker
2022-01-09  3:18     ` Rich Felker
2022-01-13 16:30       ` Alyssa Ross
2022-01-13 17:40         ` Rich Felker
2022-01-13 18:53           ` Alyssa Ross
2022-05-12 14:08             ` Rich Felker
2022-05-12 18:02               ` Alyssa Ross
2022-05-12 18:15                 ` Rich Felker
2022-05-15 18:38                   ` Alyssa Ross
2022-05-15 23:19                     ` Rich Felker
2022-05-16 10:19                       ` Alyssa Ross
2022-05-12 20:58               ` Oliver Ford
2022-05-12 21:10                 ` Rich Felker
2022-05-13 21:39                   ` Oliver Ford
2022-05-14  4:24                     ` Rich Felker
2022-05-14 22:16                       ` Oliver Ford
2022-05-15 23:31               ` Rich Felker
2022-05-16 10:07                 ` Alyssa Ross

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).