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

[PATCH v2 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>
---
Changes in v2:
- Include <sys/mman.h> for tests.
- Implement FILE* in terms of integer pointers.
- Provide fdopen() and fileno().
- Link to v1: https://lore.kernel.org/lkml/20230328-nolibc-printf-test-v1-0-d7290ec893dd@weissschuh.net/

---
Thomas Weißschuh (3):
      tools/nolibc: add wrapper for memfd_create
      tools/nolibc: implement fd-based FILE streams
      tools/nolibc: add testcases for vfprintf

 tools/include/nolibc/stdio.h                 | 60 +++++++++++----------
 tools/include/nolibc/sys.h                   | 23 ++++++++
 tools/testing/selftests/nolibc/nolibc-test.c | 78 ++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 27 deletions(-)
---
base-commit: a63baab5f60110f3631c98b55d59066f1c68c4f7
change-id: 20230328-nolibc-printf-test-052d5abc2118

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

[PATCH v2 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 v2 2/3] tools/nolibc: implement fd-based FILE streams

[Thomas Weißschuh]

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

It will be used by a future testcase.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---

Willy:

This uses intptr_t instead of uintptr_t as proposed because uintptr_t
can not be negative.
---
 tools/include/nolibc/stdio.h | 60 ++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
index 96ac8afc5aee..edf1009e9203 100644
--- a/tools/include/nolibc/stdio.h
+++ b/tools/include/nolibc/stdio.h
@@ -21,17 +21,40 @@
 #define EOF (-1)
 #endif
 
-/* just define FILE as a non-empty type */
+/* 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;
 
-/* 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*)(intptr_t)~STDIN_FILENO;
+static __attribute__((unused)) FILE* const stdout = (FILE*)(intptr_t)~STDOUT_FILENO;
+static __attribute__((unused)) FILE* const stderr = (FILE*)(intptr_t)~STDERR_FILENO;
+
+/* provides a FILE* equivalent of fd. The mode is ignored. */
+static __attribute__((unused))
+FILE *fdopen(int fd, const char *mode __attribute__((unused)))
+{
+	if (fd < 0)
+		return NULL;
+	return (FILE*)(intptr_t)~fd;
+}
+
+/* provides the fd from of stream. */
+static __attribute__((unused))
+int fileno(FILE *stream)
+{
+	intptr_t i = (intptr_t)stream;
+
+	if (i > 0) {
+		SET_ERRNO(EBADF);
+		return -1;
+	}
+	return ~i;
+}
 
 /* getc(), fgetc(), getchar() */
 
@@ -41,14 +64,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(fileno(stream), &ch, 1) <= 0)
 		return EOF;
 	return ch;
 }
@@ -68,14 +85,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(fileno(stream), &ch, 1) <= 0)
 		return EOF;
 	return ch;
 }
@@ -96,12 +107,7 @@ 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;
+	int fd = fileno(stream);
 
 	while (size) {
 		ret = write(fd, buf, size);

-- 
2.40.0

Re: [PATCH v2 2/3] tools/nolibc: implement fd-based FILE streams

[Willy Tarreau]

On Sun, Apr 02, 2023 at 01:02:46PM +0000, Thomas Weißschuh wrote:
> Willy:
> 
> This uses intptr_t instead of uintptr_t as proposed because uintptr_t
> can not be negative.

Ah yes good point.

> +/* provides the fd from of stream. */
> +static __attribute__((unused))
> +int fileno(FILE *stream)
> +{
> +	intptr_t i = (intptr_t)stream;
> +
> +	if (i > 0) {

If you don't mind I'll change this to "if (i >= 0)" since we also want
to set errno on NULL.


> +		SET_ERRNO(EBADF);
> +		return -1;
> +	}
> +	return ~i;
> +}

OK for the rest of the series.

Thanks!
Willy

Re: [PATCH v2 2/3] tools/nolibc: implement fd-based FILE streams

[Thomas Weißschuh]

On 2023-04-02 16:03:38+0200, Willy Tarreau wrote:
> On Sun, Apr 02, 2023 at 01:02:46PM +0000, Thomas Weißschuh wrote:
> > Willy:
> > 
> > This uses intptr_t instead of uintptr_t as proposed because uintptr_t
> > can not be negative.
> 
> Ah yes good point.
> 
> > +/* provides the fd from of stream. */
> > +static __attribute__((unused))
> > +int fileno(FILE *stream)
> > +{
> > +	intptr_t i = (intptr_t)stream;
> > +
> > +	if (i > 0) {
> 
> If you don't mind I'll change this to "if (i >= 0)" since we also want
> to set errno on NULL.

Sounds good.

> > +		SET_ERRNO(EBADF);
> > +		return -1;
> > +	}
> > +	return ~i;
> > +}
> 
> OK for the rest of the series.

Thanks,
Thomas

[PATCH v2 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 | 78 ++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 47013b78972e..25a64bda2722 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -20,6 +20,7 @@
 #include <linux/reboot.h>
 #include <sys/io.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/mount.h>
 #include <sys/reboot.h>
 #include <sys/stat.h>
@@ -667,6 +668,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(fdopen(fd, "w+"), 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 +851,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 v2 0/3] tools/nolibc: add testcases for vfprintf

[Willy Tarreau]

Thomas,

On Sun, Apr 02, 2023 at 01:02:44PM +0000, Thomas Weißschuh wrote:
> vfprintf() is complex and so far did not have proper tests.
> 
> This series is based on the "dev" branch of the RCU tree.

I've just ran it with glibc to see:

  $ gcc nolibc-test.c  
  $ ./a.out vfprintf
  Running test 'vfprintf'
  0 empty "" = ""                                                  [OK]
  1 simple written(3) != read(0)                                  [FAIL]
  2 string written(3) != read(0)                                  [FAIL]
  3 number written(4) != read(0)                                  [FAIL]
  4 negnumber written(5) != read(0)                               [FAIL]
  5 unsigned written(5) != read(0)                                [FAIL]
  6 char written(1) != read(0)                                    [FAIL]
  7 hex written(1) != read(0)                                     [FAIL]
  8 pointer written(5) != 3                                       [FAIL]
  Errors during this test: 8

The main issue was that glibc uses buffered writes by default.

I could fix them with fflush() (which we don't have so it required an
ifndef), and this also made me realize that we were missing an fclose()
as well for compatibility with glibc. With this it got better:

  Running test 'vfprintf'
  0 empty "" = ""                                                  [OK]
  1 simple "foo" = "foo"                                           [OK]
  2 string "foo" = "foo"                                           [OK]
  3 number "1234" = "1234"                                         [OK]
  4 negnumber "-1234" = "-1234"                                    [OK]
  5 unsigned "12345" = "12345"                                     [OK]
  6 char "c" = "c"                                                 [OK]
  7 hex "f" = "f"                                                  [OK]
  8 pointer written(5) != 3                                       [FAIL]
  Errors during this test: 1

This is caused by glibc emitting "(nil)" while we emit "0x0" for a NULL
pointer since we use the same code as when dumping integers. I could fix
that one as well by printing (void*)1 instead, which shows "0x1" for both.

This gives me the patch below on top of yours, which I think would make
sense to integrate in this form or a simplified one if we manage to add
fflush() and fclose() earlier.

What do you think ?

Thanks,
Willy


diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 28a8d77078dc..2958dc3eca93 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -678,6 +678,7 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
 	int ret, fd, w, r;
 	char buf[100];
 	va_list args;
+	FILE *memfile;
 
 	fd = memfd_create("vfprintf", 0);
 	if (fd == -1) {
@@ -685,8 +686,14 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
 		return 1;
 	}
 
+	memfile = fdopen(fd, "w+");
+	if (!memfile) {
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
 	va_start(args, fmt);
-	w = vfprintf(fdopen(fd, "w+"), fmt, args);
+	w = vfprintf(memfile, fmt, args);
 	va_end(args);
 
 	if (w != c) {
@@ -695,12 +702,19 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
 		return 1;
 	}
 
+#ifndef _NOLIBC_STDIO_H
+	fflush(memfile);
+#endif
 	lseek(fd, 0, SEEK_SET);
 
 	r = read(fd, buf, sizeof(buf) - 1);
 	buf[r] = '\0';
 
+#ifndef _NOLIBC_STDIO_H
+	fclose(memfile);
+#else
 	close(fd);
+#endif
 
 	if (r != w) {
 		llen += printf(" written(%d) != read(%d)", w, r);
@@ -737,7 +751,7 @@ static int run_vfprintf(int min, int max)
 		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_TEST(pointer);      EXPECT_VFPRINTF(3, "0x1", "%p", (void*)0x1); break;
 		case __LINE__:
 			return ret; /* must be last */
 		/* note: do not set any defaults so as to permit holes above */

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

[Thomas Weißschuh]

On 2023-04-02 18:43:21+0200, Willy Tarreau wrote:
> Thomas,
> 
> On Sun, Apr 02, 2023 at 01:02:44PM +0000, Thomas Weißschuh wrote:
> > vfprintf() is complex and so far did not have proper tests.
> > 
> > This series is based on the "dev" branch of the RCU tree.
> 
> I've just ran it with glibc to see:
> 
>   $ gcc nolibc-test.c  
>   $ ./a.out vfprintf
>   Running test 'vfprintf'
>   0 empty "" = ""                                                  [OK]
>   1 simple written(3) != read(0)                                  [FAIL]
>   2 string written(3) != read(0)                                  [FAIL]
>   3 number written(4) != read(0)                                  [FAIL]
>   4 negnumber written(5) != read(0)                               [FAIL]
>   5 unsigned written(5) != read(0)                                [FAIL]
>   6 char written(1) != read(0)                                    [FAIL]
>   7 hex written(1) != read(0)                                     [FAIL]
>   8 pointer written(5) != 3                                       [FAIL]
>   Errors during this test: 8
> 
> The main issue was that glibc uses buffered writes by default.
> 
> I could fix them with fflush() (which we don't have so it required an
> ifndef), and this also made me realize that we were missing an fclose()
> as well for compatibility with glibc. With this it got better:
> 
>   Running test 'vfprintf'
>   0 empty "" = ""                                                  [OK]
>   1 simple "foo" = "foo"                                           [OK]
>   2 string "foo" = "foo"                                           [OK]
>   3 number "1234" = "1234"                                         [OK]
>   4 negnumber "-1234" = "-1234"                                    [OK]
>   5 unsigned "12345" = "12345"                                     [OK]
>   6 char "c" = "c"                                                 [OK]
>   7 hex "f" = "f"                                                  [OK]
>   8 pointer written(5) != 3                                       [FAIL]
>   Errors during this test: 1
> 
> This is caused by glibc emitting "(nil)" while we emit "0x0" for a NULL
> pointer since we use the same code as when dumping integers. I could fix
> that one as well by printing (void*)1 instead, which shows "0x1" for both.
> 
> This gives me the patch below on top of yours, which I think would make
> sense to integrate in this form or a simplified one if we manage to add
> fflush() and fclose() earlier.
> 
> What do you think ?
> 
> Thanks,
> Willy
> 
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 28a8d77078dc..2958dc3eca93 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -678,6 +678,7 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  	int ret, fd, w, r;
>  	char buf[100];
>  	va_list args;
> +	FILE *memfile;
>  
>  	fd = memfd_create("vfprintf", 0);
>  	if (fd == -1) {
> @@ -685,8 +686,14 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  		return 1;
>  	}
>  
> +	memfile = fdopen(fd, "w+");
> +	if (!memfile) {
> +		pad_spc(llen, 64, "[FAIL]\n");
> +		return 1;
> +	}
> +
>  	va_start(args, fmt);
> -	w = vfprintf(fdopen(fd, "w+"), fmt, args);
> +	w = vfprintf(memfile, fmt, args);
>  	va_end(args);
>  
>  	if (w != c) {
> @@ -695,12 +702,19 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char
>  		return 1;
>  	}
>  
> +#ifndef _NOLIBC_STDIO_H
> +	fflush(memfile);
> +#endif
>  	lseek(fd, 0, SEEK_SET);
>  
>  	r = read(fd, buf, sizeof(buf) - 1);
>  	buf[r] = '\0';
>  
> +#ifndef _NOLIBC_STDIO_H
> +	fclose(memfile);
> +#else
>  	close(fd);
> +#endif

Wouldn't it be nicer to implement fflush/fclose in nolibc?
I can send a v3 with that.

>  	if (r != w) {
>  		llen += printf(" written(%d) != read(%d)", w, r);
> @@ -737,7 +751,7 @@ static int run_vfprintf(int min, int max)
>  		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_TEST(pointer);      EXPECT_VFPRINTF(3, "0x1", "%p", (void*)0x1); break;
>  		case __LINE__:
>  			return ret; /* must be last */
>  		/* note: do not set any defaults so as to permit holes above */

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

[Willy Tarreau]

On Sun, Apr 02, 2023 at 05:17:53PM +0000, Thomas Weißschuh wrote:
> Wouldn't it be nicer to implement fflush/fclose in nolibc?

Yes that was my point, I just didn't know whether you still had some
time left to assign to this or not, hence my question.

> I can send a v3 with that.

Its currently running tests here. It looks pretty good, I guess I'm
going to bless it.

Thanks!
Willy