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

[PATCH v3 0/4] 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 v3:
- also provide and use fflush/fclose.
- reject fileno(NULL).
- provide compatability with buffered streams from glibc.
- Link to v2: https://lore.kernel.org/r/20230328-nolibc-printf-test-v2-0-f72bdf210190@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 (4):
      tools/nolibc: add libc-test binary
      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                 | 95 ++++++++++++++++++++--------
 tools/include/nolibc/sys.h                   | 23 +++++++
 tools/testing/selftests/nolibc/.gitignore    |  1 +
 tools/testing/selftests/nolibc/Makefile      |  5 ++
 tools/testing/selftests/nolibc/nolibc-test.c | 86 +++++++++++++++++++++++++
 5 files changed, 183 insertions(+), 27 deletions(-)
---
base-commit: a63baab5f60110f3631c98b55d59066f1c68c4f7
change-id: 20230328-nolibc-printf-test-052d5abc2118

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

[PATCH v3 1/4] tools/nolibc: add libc-test binary

[Thomas Weißschuh]

This can be used to easily compare the behavior of nolibc to the system
libc.

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

---

Feel free to drop this if it does not fit in.
---
 tools/testing/selftests/nolibc/.gitignore | 1 +
 tools/testing/selftests/nolibc/Makefile   | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/tools/testing/selftests/nolibc/.gitignore b/tools/testing/selftests/nolibc/.gitignore
index 4696df589d68..52f613cdad54 100644
--- a/tools/testing/selftests/nolibc/.gitignore
+++ b/tools/testing/selftests/nolibc/.gitignore
@@ -1,4 +1,5 @@
 /initramfs/
+/libc-test
 /nolibc-test
 /run.out
 /sysroot/
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index bbce57420465..903990d22ac8 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -128,6 +128,9 @@ nolibc-test: nolibc-test.c sysroot/$(ARCH)/include
 	$(QUIET_CC)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ \
 	  -nostdlib -static -Isysroot/$(ARCH)/include $< -lgcc
 
+libc-test: nolibc-test.c
+	$(QUIET_CC)$(CC) -o $@ $<
+
 # qemu user-land test
 run-user: nolibc-test
 	$(Q)qemu-$(QEMU_ARCH) ./nolibc-test > "$(CURDIR)/run.out" || :
@@ -159,6 +162,8 @@ clean:
 	$(Q)rm -rf sysroot
 	$(call QUIET_CLEAN, nolibc-test)
 	$(Q)rm -f nolibc-test
+	$(call QUIET_CLEAN, libc-test)
+	$(Q)rm -f libc-test
 	$(call QUIET_CLEAN, initramfs)
 	$(Q)rm -rf initramfs
 	$(call QUIET_CLEAN, run.out)

-- 
2.40.0

Re: [PATCH v3 1/4] tools/nolibc: add libc-test binary

[Willy Tarreau]

On Sun, Apr 02, 2023 at 06:04:34PM +0000, Thomas Weißschuh wrote:
> This can be used to easily compare the behavior of nolibc to the system
> libc.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> Feel free to drop this if it does not fit in.

On the opposite! I wanted to do it, you beat me to it. I'm just going to
add an entry for it in the help message.

Thank you!
Willy

[PATCH v3 2/4] 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 v3 3/4] 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 | 95 +++++++++++++++++++++++++++++++-------------
 1 file changed, 68 insertions(+), 27 deletions(-)

diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
index 96ac8afc5aee..4add736c07aa 100644
--- a/tools/include/nolibc/stdio.h
+++ b/tools/include/nolibc/stdio.h
@@ -21,17 +21,75 @@
 #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) {
+		SET_ERRNO(EBADF);
+		return NULL;
+	}
+	return (FILE*)(intptr_t)~fd;
+}
+
+/* provides the fd 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;
+}
+
+/* flush a stream. */
+static __attribute__((unused))
+int fflush(FILE *stream)
+{
+	intptr_t i = (intptr_t)stream;
+
+	/* NULL is valid here. */
+	if (i > 0) {
+		SET_ERRNO(EBADF);
+		return -1;
+	}
+
+	/* Don't do anything, nolibc does not support buffering. */
+	return 0;
+}
+
+/* flush a stream. */
+static __attribute__((unused))
+int fclose(FILE *stream)
+{
+	intptr_t i = (intptr_t)stream;
+
+	if (i >= 0) {
+		SET_ERRNO(EBADF);
+		return -1;
+	}
+
+	if (close(~i))
+		return EOF;
+
+	return 0;
+}
 
 /* getc(), fgetc(), getchar() */
 
@@ -41,14 +99,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 +120,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 +142,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 v3 3/4] tools/nolibc: implement fd-based FILE streams

[Mark Brown]

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

On Sun, Apr 02, 2023 at 06:04:36PM +0000, Thomas Weißschuh wrote:

> This enables the usage of the stream APIs with arbitrary filedescriptors.
> 
> It will be used by a future testcase.

This breaks the explicit use of standard streams:

> -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;

Nothing in this change (or anything else in the series AFAICT) causes
STDx_FILENO to be declared so we get errors like below in -next when a
kselftest is built with this version of nolibc:

clang --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument --target=aarch64-linux-gnu -fintegrated-as -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib \
	-include ../../../../include/nolibc/nolibc.h -I../..\
	-static -ffreestanding -Wall za-fork.c /tmp/kci/linux/build/kselftest/arm64/fp/za-fork-asm.o -o /tmp/kci/linux/build/kselftest/arm64/fp/za-fork
In file included from <built-in>:1:
In file included from ./../../../../include/nolibc/nolibc.h:97:
In file included from ./../../../../include/nolibc/arch.h:25:
./../../../../include/nolibc/arch-aarch64.h:176:35: warning: unknown attribute 'optimize' ignored [-Wunknown-attributes]
void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from <built-in>:1:
In file included from ./../../../../include/nolibc/nolibc.h:102:
./../../../../include/nolibc/stdio.h:33:71: error: use of undeclared identifier 'STDIN_FILENO'
static __attribute__((unused)) FILE* const stdin  = (FILE*)(intptr_t)~STDIN_FILENO;
                                                                      ^
./../../../../include/nolibc/stdio.h:34:71: error: use of undeclared identifier 'STDOUT_FILENO'
static __attribute__((unused)) FILE* const stdout = (FILE*)(intptr_t)~STDOUT_FILENO;
                                                                      ^
./../../../../include/nolibc/stdio.h:35:71: error: use of undeclared identifier 'STDERR_FILENO'
static __attribute__((unused)) FILE* const stderr = (FILE*)(intptr_t)~STDERR_FILENO;
                                                                      ^
1 warning and 3 errors generated.

The kselftest branch itself is fine, the issues manifest when it is
merged with the nolibc branch.  I'm not quite sure what the implicit
include that's been missed here is?

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

Re: [PATCH v3 3/4] tools/nolibc: implement fd-based FILE streams

[linux]

Hi Mark,


Apr 12, 2023 17:58:45 Mark Brown <broonie@kernel.org>:

> On Sun, Apr 02, 2023 at 06:04:36PM +0000, Thomas Weißschuh wrote:
>
>> This enables the usage of the stream APIs with arbitrary filedescriptors.
>>
>> It will be used by a future testcase.
>
> This breaks the explicit use of standard streams:
>
>> -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;
>
> Nothing in this change (or anything else in the series AFAICT) causes
> STDx_FILENO to be declared so we get errors like below in -next when a
> kselftest is built with this version of nolibc:

These definitions come from
"tools/nolibc: add definitions for standard fds".
This patch was part of the nolibc stack protector series which is older than this series and went through the same channels.
So I'm not sure how one series made it into next and the other didn't.

This would also have been noticed by Willy and Paul running their tests.

>
> clang --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument --target=aarch64-linux-gnu -fintegrated-as -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib \
>     -include ../../../../include/nolibc/nolibc.h -I../..\
>     -static -ffreestanding -Wall za-fork.c /tmp/kci/linux/build/kselftest/arm64/fp/za-fork-asm.o -o /tmp/kci/linux/build/kselftest/arm64/fp/za-fork
> In file included from <built-in>:1:
> In file included from ./../../../../include/nolibc/nolibc.h:97:
> In file included from ./../../../../include/nolibc/arch.h:25:
> ./../../../../include/nolibc/arch-aarch64.h:176:35: warning: unknown attribute 'optimize' ignored [-Wunknown-attributes]
> void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
>                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from <built-in>:1:
> In file included from ./../../../../include/nolibc/nolibc.h:102:
> ./../../../../include/nolibc/stdio.h:33:71: error: use of undeclared identifier 'STDIN_FILENO'
> static __attribute__((unused)) FILE* const stdin  = (FILE*)(intptr_t)~STDIN_FILENO;
>                                                                       ^
> ./../../../../include/nolibc/stdio.h:34:71: error: use of undeclared identifier 'STDOUT_FILENO'
> static __attribute__((unused)) FILE* const stdout = (FILE*)(intptr_t)~STDOUT_FILENO;
>                                                                       ^
> ./../../../../include/nolibc/stdio.h:35:71: error: use of undeclared identifier 'STDERR_FILENO'
> static __attribute__((unused)) FILE* const stderr = (FILE*)(intptr_t)~STDERR_FILENO;
>                                                                       ^
> 1 warning and 3 errors generated.
>
> The kselftest branch itself is fine, the issues manifest when it is
> merged with the nolibc branch.  I'm not quite sure what the implicit> include that's been missed here is?

These defines should be available to all users of nolibc.
It seems more of an issue with the patchsets going through different trees.

I can investigate this more tomorrow with my proper development setup.

Thomas

Re: [PATCH v3 3/4] tools/nolibc: implement fd-based FILE streams

[Mark Brown]

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

On Thu, Apr 13, 2023 at 03:09:27PM +0200, linux@weissschuh.net wrote:
> Apr 12, 2023 17:58:45 Mark Brown <broonie@kernel.org>:

> > Nothing in this change (or anything else in the series AFAICT) causes
> > STDx_FILENO to be declared so we get errors like below in -next when a
> > kselftest is built with this version of nolibc:

> These definitions come from
> "tools/nolibc: add definitions for standard fds".
> This patch was part of the nolibc stack protector series which is older than this series and went through the same channels.
> So I'm not sure how one series made it into next and the other didn't.

> This would also have been noticed by Willy and Paul running their tests.

Hrm, that commit is actually in -next and Paul's pull request, not sure
why it wasn't showing up in greps.  The issue is that you've added a
dependency from nolibc's stdio.h to unistd.h but nolibc.h includes
unistd.h last and there's no other include, meaning that at the time
that stdio.h is compiled there's no definition of the constants visible.

The below fixes the issue, I'll submit it properly later today:

diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index 04739a6293c4..05a228a6ee78 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -99,11 +99,11 @@
 #include "sys.h"
 #include "ctype.h"
 #include "signal.h"
+#include "unistd.h"
 #include "stdio.h"
 #include "stdlib.h"
 #include "string.h"
 #include "time.h"
-#include "unistd.h"
 #include "stackprotector.h"
 
 /* Used by programs to avoid std includes */

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

[PATCH v3 4/4] 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 | 86 ++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 47013b78972e..de943e028933 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,90 @@ 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];
+	FILE *memfile;
+	va_list args;
+
+	fd = memfd_create("vfprintf", 0);
+	if (fd == -1) {
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	memfile = fdopen(fd, "w+");
+	if (!memfile) {
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	va_start(args, fmt);
+	w = vfprintf(memfile, fmt, args);
+	va_end(args);
+
+	if (w != c) {
+		llen += printf(" written(%d) != %d", w, (int) c);
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+	}
+
+	fflush(memfile);
+	lseek(fd, 0, SEEK_SET);
+
+	r = read(fd, buf, sizeof(buf) - 1);
+	buf[r] = '\0';
+
+	fclose(memfile);
+
+	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, "0x1", "%p", (void *) 0x1); 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 +859,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 v3 0/4] tools/nolibc: add testcases for vfprintf

[Willy Tarreau]

On Sun, Apr 02, 2023 at 06:04:33PM +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.

So just to confirm, it's all good, thank you very much. I've passed it
to Paul.

Have a nice end of week-end ;-)
Willy