[PATCH 0/3] tools/nolibc: add testcases for vfprintf

[PATCH 0/3] tools/nolibc: add testcases for vfprintf

[Thomas Weißschuh]

vfprintf() is complex and so far did not have proper tests.

This series is based on the "dev" branch of the RCU tree.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (3):
      tools/nolibc: add wrapper for memfd_create
      tools/nolibc: let FILE streams contain an fd
      tools/nolibc: add testcases for vfprintf

 tools/include/nolibc/stdio.h                 | 36 +++----------
 tools/include/nolibc/sys.h                   | 23 +++++++++
 tools/testing/selftests/nolibc/nolibc-test.c | 77 ++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 29 deletions(-)
---
base-commit: a5333c037de823912dd20e933785c63de7679e64
change-id: 20230328-nolibc-printf-test-052d5abc2118

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/3] tools/nolibc: add wrapper for memfd_create

[Thomas Weißschuh]

This is useful for users and will also be used by a future testcase.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/sys.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 5d624dc63a42..bea9760dbd16 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -1365,6 +1365,29 @@ ssize_t write(int fd, const void *buf, size_t count)
 	return ret;
 }
 
+
+/*
+ * int memfd_create(const char *name, unsigned int flags);
+ */
+
+static __attribute__((unused))
+int sys_memfd_create(const char *name, unsigned int flags)
+{
+	return my_syscall2(__NR_memfd_create, name, flags);
+}
+
+static __attribute__((unused))
+int memfd_create(const char *name, unsigned int flags)
+{
+	ssize_t ret = sys_memfd_create(name, flags);
+
+	if (ret < 0) {
+		SET_ERRNO(-ret);
+		ret = -1;
+	}
+	return ret;
+}
+
 /* make sure to include all global symbols */
 #include "nolibc.h"
 

-- 
2.40.0

[PATCH 2/3] tools/nolibc: let FILE streams contain an fd

[Thomas Weißschuh]

This enables the usage of the stream APIs with arbitrary filedescriptors.

It will be used by a future testcase.
Users can also use nolibc-specific code to do the same.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/stdio.h | 36 +++++++-----------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
index 96ac8afc5aee..cb58912b98e5 100644
--- a/tools/include/nolibc/stdio.h
+++ b/tools/include/nolibc/stdio.h
@@ -21,17 +21,13 @@
 #define EOF (-1)
 #endif
 
-/* just define FILE as a non-empty type */
 typedef struct FILE {
-	char dummy[1];
+	int fd;
 } FILE;
 
-/* We define the 3 common stdio files as constant invalid pointers that
- * are easily recognized.
- */
-static __attribute__((unused)) FILE* const stdin  = (FILE*)-3;
-static __attribute__((unused)) FILE* const stdout = (FILE*)-2;
-static __attribute__((unused)) FILE* const stderr = (FILE*)-1;
+static __attribute__((unused)) FILE* const stdin  = &(FILE){ STDIN_FILENO  };
+static __attribute__((unused)) FILE* const stdout = &(FILE){ STDOUT_FILENO };
+static __attribute__((unused)) FILE* const stderr = &(FILE){ STDERR_FILENO };
 
 /* getc(), fgetc(), getchar() */
 
@@ -41,14 +37,8 @@ static __attribute__((unused))
 int fgetc(FILE* stream)
 {
 	unsigned char ch;
-	int fd;
 
-	if (stream < stdin || stream > stderr)
-		return EOF;
-
-	fd = 3 + (long)stream;
-
-	if (read(fd, &ch, 1) <= 0)
+	if (read(stream->fd, &ch, 1) <= 0)
 		return EOF;
 	return ch;
 }
@@ -68,14 +58,8 @@ static __attribute__((unused))
 int fputc(int c, FILE* stream)
 {
 	unsigned char ch = c;
-	int fd;
 
-	if (stream < stdin || stream > stderr)
-		return EOF;
-
-	fd = 3 + (long)stream;
-
-	if (write(fd, &ch, 1) <= 0)
+	if (write(stream->fd, &ch, 1) <= 0)
 		return EOF;
 	return ch;
 }
@@ -96,15 +80,9 @@ static __attribute__((unused))
 int _fwrite(const void *buf, size_t size, FILE *stream)
 {
 	ssize_t ret;
-	int fd;
-
-	if (stream < stdin || stream > stderr)
-		return EOF;
-
-	fd = 3 + (long)stream;
 
 	while (size) {
-		ret = write(fd, buf, size);
+		ret = write(stream->fd, buf, size);
 		if (ret <= 0)
 			return EOF;
 		size -= ret;

-- 
2.40.0

Re: [PATCH 2/3] tools/nolibc: let FILE streams contain an fd

[Willy Tarreau]

Hi Thomas,

On Tue, Mar 28, 2023 at 09:01:30PM +0000, Thomas Weißschuh wrote:
> This enables the usage of the stream APIs with arbitrary filedescriptors.
> 
> It will be used by a future testcase.
> Users can also use nolibc-specific code to do the same.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  tools/include/nolibc/stdio.h | 36 +++++++-----------------------------
>  1 file changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index 96ac8afc5aee..cb58912b98e5 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
> @@ -21,17 +21,13 @@
>  #define EOF (-1)
>  #endif
>  
> -/* just define FILE as a non-empty type */
>  typedef struct FILE {
> -	char dummy[1];
> +	int fd;
>  } FILE;

In my opinion this makes the usage of FILE* more complicated than before.
Look for example at your vfprintf() test case, you had to do this with
the fd provided by memfd_create():

        w = vfprintf(&(FILE) { fd }, fmt, args);

This is particularly ugly especially for code that needs to be exposed
to end-user. Also it breaks compatibility with glibc that is seldom used
to check if trouble comes from nolibc or from the test itself. It would
be much better to have fdopen() here but the new struct makes this
impossible.

I would propose instead to go back to the previous definition and simply
change its semantics a little bit:

   /* just define FILE as a non-empty type. The value of the pointer gives
    * the FD: FILE=~fd for fd>=0 or NULL for fd<0. This way positive FILE
    * are immediately identified as abnormal entries (i.e. possible copies
    * of valid pointers to something else).
   typedef struct FILE {
   	char dummy[1];
   } FILE;

Then we can have:

  static __attribute__((unused)) FILE* const stdin  = (FILE*)(uintptr_t)~STDIN_FILENO;
  static __attribute__((unused)) FILE* const stdout = (FILE*)(uintptr_t)~STDOUT_FILENO;
  static __attribute__((unused)) FILE* const stderr = (FILE*)(uintptr_t)~STDERR_FILENO;

  /* provides a FILE* equivalent of fd. The mode is ignored. */
  static __attribute__((unused))
  FILE *fdopen(int fd, const char *mode)
  {
  	if (fd < 0)
  		return NULL;
  	return (FILE*)(uintptr_t)~fd;
  }

And your FD can simply be passed this way:

  fd = memfd_create("vfprintf", 0);
  if (fd == -1) {
          pad_spc(llen, 64, "[FAIL]\n");
          return 1;
  }

  va_start(args, fmt);
  w = vfprintf(fdopen(fd, "w+"), fmt, args);
  va_end(args);

This way it works more like common usage and restores glibc compatibility.

Regards,
Willy

Re: [PATCH 2/3] tools/nolibc: let FILE streams contain an fd

[Thomas Weißschuh]

On 2023-04-02 09:45:57+0200, Willy Tarreau wrote:
> Hi Thomas,
> 
> On Tue, Mar 28, 2023 at 09:01:30PM +0000, Thomas Weißschuh wrote:
> > This enables the usage of the stream APIs with arbitrary filedescriptors.
> > 
> > It will be used by a future testcase.
> > Users can also use nolibc-specific code to do the same.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  tools/include/nolibc/stdio.h | 36 +++++++-----------------------------
> >  1 file changed, 7 insertions(+), 29 deletions(-)
> > 
> > diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> > index 96ac8afc5aee..cb58912b98e5 100644
> > --- a/tools/include/nolibc/stdio.h
> > +++ b/tools/include/nolibc/stdio.h
> > @@ -21,17 +21,13 @@
> >  #define EOF (-1)
> >  #endif
> >  
> > -/* just define FILE as a non-empty type */
> >  typedef struct FILE {
> > -	char dummy[1];
> > +	int fd;
> >  } FILE;
> 
> In my opinion this makes the usage of FILE* more complicated than before.
> Look for example at your vfprintf() test case, you had to do this with
> the fd provided by memfd_create():
> 
>         w = vfprintf(&(FILE) { fd }, fmt, args);
> 
> This is particularly ugly especially for code that needs to be exposed
> to end-user. Also it breaks compatibility with glibc that is seldom used
> to check if trouble comes from nolibc or from the test itself. It would
> be much better to have fdopen() here but the new struct makes this
> impossible.

My original reasoning was to avoiding to implement a version of fdopen()
that does not properly check the "mode" argument.

But rereading the manpage of fdopen() we don't actually need to check
them.

Also the goal was for this "API" to be completely opaque to the enduser
and only used internally in nolibc(-test).
But building the tests with glibc is indeed a great usecase.

fdopen() it is.

Also I think I'll extend the -std=c99/-std=gnu89 series to also build
the test against glibc to prevent me from breaking this in the future.

> I would propose instead to go back to the previous definition and simply
> change its semantics a little bit:
> 
>    /* just define FILE as a non-empty type. The value of the pointer gives
>     * the FD: FILE=~fd for fd>=0 or NULL for fd<0. This way positive FILE
>     * are immediately identified as abnormal entries (i.e. possible copies
>     * of valid pointers to something else).
>    typedef struct FILE {
>    	char dummy[1];
>    } FILE;
> 
> Then we can have:
> 
>   static __attribute__((unused)) FILE* const stdin  = (FILE*)(uintptr_t)~STDIN_FILENO;
>   static __attribute__((unused)) FILE* const stdout = (FILE*)(uintptr_t)~STDOUT_FILENO;
>   static __attribute__((unused)) FILE* const stderr = (FILE*)(uintptr_t)~STDERR_FILENO;
> 
>   /* provides a FILE* equivalent of fd. The mode is ignored. */
>   static __attribute__((unused))
>   FILE *fdopen(int fd, const char *mode)
>   {
>   	if (fd < 0)
>   		return NULL;
>   	return (FILE*)(uintptr_t)~fd;
>   }
> 
> And your FD can simply be passed this way:
> 
>   fd = memfd_create("vfprintf", 0);
>   if (fd == -1) {
>           pad_spc(llen, 64, "[FAIL]\n");
>           return 1;
>   }
> 
>   va_start(args, fmt);
>   w = vfprintf(fdopen(fd, "w+"), fmt, args);
>   va_end(args);
> 
> This way it works more like common usage and restores glibc compatibility.

Ack, will do.

[PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Thomas Weißschuh]

vfprintf() is complex and so far did not have proper tests.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 77 ++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 47013b78972e..cc60c0f7363d 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -667,6 +667,82 @@ int run_stdlib(int min, int max)
 	return ret;
 }
 
+#define EXPECT_VFPRINTF(c, expected, fmt, ...)				\
+	ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
+
+static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...)
+{
+	int ret, fd, w, r;
+	char buf[100];
+	va_list args;
+
+	fd = memfd_create("vfprintf", 0);
+	if (fd == -1) {
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	va_start(args, fmt);
+	w = vfprintf(&(FILE) { fd }, fmt, args);
+	va_end(args);
+
+	if (w != c) {
+		llen += printf(" written(%d) != %d", w, (int) c);
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	lseek(fd, 0, SEEK_SET);
+
+	r = read(fd, buf, sizeof(buf) - 1);
+	buf[r] = '\0';
+
+	close(fd);
+
+	if (r != w) {
+		llen += printf(" written(%d) != read(%d)", w, r);
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	llen += printf(" \"%s\" = \"%s\"", expected, buf);
+	ret = strncmp(expected, buf, c);
+
+	pad_spc(llen, 64, ret ? "[FAIL]\n" : " [OK]\n");
+	return ret;
+}
+
+static int run_vfprintf(int min, int max)
+{
+	int test;
+	int tmp;
+	int ret = 0;
+	void *p1, *p2;
+
+	for (test = min; test >= 0 && test <= max; test++) {
+		int llen = 0; // line length
+
+		/* avoid leaving empty lines below, this will insert holes into
+		 * test numbers.
+		 */
+		switch (test + __LINE__ + 1) {
+		CASE_TEST(empty);        EXPECT_VFPRINTF(0, "", ""); break;
+		CASE_TEST(simple);       EXPECT_VFPRINTF(3, "foo", "foo"); break;
+		CASE_TEST(string);       EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break;
+		CASE_TEST(number);       EXPECT_VFPRINTF(4, "1234", "%d", 1234); break;
+		CASE_TEST(negnumber);    EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break;
+		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
+		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
+		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
+		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break;
+		case __LINE__:
+			return ret; /* must be last */
+		/* note: do not set any defaults so as to permit holes above */
+		}
+	}
+	return ret;
+}
+
 static int smash_stack(void)
 {
 	char buf[100];
@@ -774,6 +850,7 @@ static const struct test test_names[] = {
 	/* add new tests here */
 	{ .name = "syscall",    .func = run_syscall    },
 	{ .name = "stdlib",     .func = run_stdlib     },
+	{ .name = "vfprintf",   .func = run_vfprintf   },
 	{ .name = "protection", .func = run_protection },
 	{ 0 }
 };

-- 
2.40.0

Re: [PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Willy Tarreau]

On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote:
> vfprintf() is complex and so far did not have proper tests.

This is an excellent idea, I totally agree, and I wouldn't be surprised
if there were still bugs there.

> +		switch (test + __LINE__ + 1) {
> +		CASE_TEST(empty);        EXPECT_VFPRINTF(0, "", ""); break;
> +		CASE_TEST(simple);       EXPECT_VFPRINTF(3, "foo", "foo"); break;
> +		CASE_TEST(string);       EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break;
> +		CASE_TEST(number);       EXPECT_VFPRINTF(4, "1234", "%d", 1234); break;
> +		CASE_TEST(negnumber);    EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break;
> +		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
> +		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
> +		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
> +		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break;

I don't see a reason why not to move them to the stdlib category, since
these tests are there to validate that the libc-provided functions do
work. Maybe you intended to further extend it ? In this case maybe we
could move that to an "stdio" category then but I'd rather avoid having
one category per function or it will quickly become annoying to select
groups of tests. So let's just prefix these test names with "printf_"
and either merge them with "stdlib" or name the category "stdio", as
you prefer.

Thank you!
Willy

Re: [PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Thomas Weißschuh]

On 2023-04-02 09:51:10+0200, Willy Tarreau wrote:
> On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote:
> > vfprintf() is complex and so far did not have proper tests.
> 
> This is an excellent idea, I totally agree, and I wouldn't be surprised
> if there were still bugs there.

The first issue I experienced was that

printf("%*s", 1, "foo") would segfault because it ignored the '*' and
just tried to interpret the number "1" as string.
When looking for the supported features of the printf implementation
there were no examples.

And before I try to add code to handle this case better I really want
some testcases.

> > +		switch (test + __LINE__ + 1) {
> > +		CASE_TEST(empty);        EXPECT_VFPRINTF(0, "", ""); break;
> > +		CASE_TEST(simple);       EXPECT_VFPRINTF(3, "foo", "foo"); break;
> > +		CASE_TEST(string);       EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break;
> > +		CASE_TEST(number);       EXPECT_VFPRINTF(4, "1234", "%d", 1234); break;
> > +		CASE_TEST(negnumber);    EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break;
> > +		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
> > +		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
> > +		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
> > +		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break;
> 
> I don't see a reason why not to move them to the stdlib category, since
> these tests are there to validate that the libc-provided functions do
> work. Maybe you intended to further extend it ? In this case maybe we
> could move that to an "stdio" category then but I'd rather avoid having
> one category per function or it will quickly become annoying to select
> groups of tests. So let's just prefix these test names with "printf_"
> and either merge them with "stdlib" or name the category "stdio", as
> you prefer.

The idea was that printf is its own very special beast that alone is
more complex than many other things combined.
When working on it, it would be useful to only run the relevant tests
without having to manually count testcase numbers.

I don't expect other single functions getting their own category.

If you still prefer to put it somewhere else I can do that, too.

Re: [PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Willy Tarreau]

On Sun, Apr 02, 2023 at 12:18:29PM +0000, Thomas Weißschuh wrote:
> On 2023-04-02 09:51:10+0200, Willy Tarreau wrote:
> > On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote:
> > > vfprintf() is complex and so far did not have proper tests.
> > 
> > This is an excellent idea, I totally agree, and I wouldn't be surprised
> > if there were still bugs there.
> 
> The first issue I experienced was that
> 
> printf("%*s", 1, "foo") would segfault because it ignored the '*' and
> just tried to interpret the number "1" as string.

Yes indeed, much like many older printf() implementations as well BTW,
that's a common issue when you try to write portable code ;-)

> When looking for the supported features of the printf implementation
> there were no examples.

Indeed!

> And before I try to add code to handle this case better I really want
> some testcases.
> 
> > > +		switch (test + __LINE__ + 1) {
> > > +		CASE_TEST(empty);        EXPECT_VFPRINTF(0, "", ""); break;
> > > +		CASE_TEST(simple);       EXPECT_VFPRINTF(3, "foo", "foo"); break;
> > > +		CASE_TEST(string);       EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break;
> > > +		CASE_TEST(number);       EXPECT_VFPRINTF(4, "1234", "%d", 1234); break;
> > > +		CASE_TEST(negnumber);    EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break;
> > > +		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
> > > +		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
> > > +		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
> > > +		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break;
> > 
> > I don't see a reason why not to move them to the stdlib category, since
> > these tests are there to validate that the libc-provided functions do
> > work. Maybe you intended to further extend it ? In this case maybe we
> > could move that to an "stdio" category then but I'd rather avoid having
> > one category per function or it will quickly become annoying to select
> > groups of tests. So let's just prefix these test names with "printf_"
> > and either merge them with "stdlib" or name the category "stdio", as
> > you prefer.
> 
> The idea was that printf is its own very special beast that alone is
> more complex than many other things combined.
> When working on it, it would be useful to only run the relevant tests
> without having to manually count testcase numbers.
> 
> I don't expect other single functions getting their own category.
> 
> If you still prefer to put it somewhere else I can do that, too.

OK, I can understand, it makes sense to some extents. And I agree that
if we'd ever extend printf it would be needed to extend these tests.
Then let's leave it that way.

Thanks,
Willy

Re: [PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Willy Tarreau]

On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote:
> +	fd = memfd_create("vfprintf", 0);
> +	if (fd == -1) {
> +		pad_spc(llen, 64, "[FAIL]\n");
> +		return 1;
> +	}

Also for this you'll need to include <sys/mman.h> in the part where nolibc
is not detected so that it continues to work with regular libcs (at least
glibc so that we have one reference to compare against).

Thanks,
Willy

Re: [PATCH 3/3] tools/nolibc: add testcases for vfprintf

[Thomas Weißschuh]

On 2023-04-02 10:00:05+0200, Willy Tarreau wrote:
> On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote:
> > +	fd = memfd_create("vfprintf", 0);
> > +	if (fd == -1) {
> > +		pad_spc(llen, 64, "[FAIL]\n");
> > +		return 1;
> > +	}
> 
> Also for this you'll need to include <sys/mman.h> in the part where nolibc
> is not detected so that it continues to work with regular libcs (at least
> glibc so that we have one reference to compare against).

Ack.