[PATCH 0/2] tools/nolibc: add pipe() and its testcase

[PATCH 0/2] tools/nolibc: add pipe() and its testcase

[Yuan Tan]

Hi,

Zhangjin and I are working on a tiny shell with nolibc. This patch
enables the missing pipe() with its testcase.

Thanks.

Yuan Tan (2):
  tools/nolibc: add pipe() support
  selftests/nolibc: add testcase for pipe.

 tools/include/nolibc/sys.h                   | 17 ++++++++++
 tools/testing/selftests/nolibc/nolibc-test.c | 34 ++++++++++++++++++++
 2 files changed, 51 insertions(+)

-- 
2.39.2

[PATCH 1/2] tools/nolibc: add pipe() support

[Yuan Tan]

pipe is crucial for shell.

Signed-off-by: Yuan Tan <tanyuan@tinylab.org>
---
 tools/include/nolibc/sys.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 8bfe7db20b80..09841fc266fe 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -752,6 +752,23 @@ int open(const char *path, int flags, ...)
 }
 
 
+/*
+ * int pipe(int pipefd[2]);
+ */
+
+static __attribute__((unused))
+int sys_pipe(int pipefd[2])
+{
+	return my_syscall1(__NR_pipe, pipefd);
+}
+
+static __attribute__((unused))
+int pipe(int pipefd[2])
+{
+	return __sysret(sys_pipe(pipefd));
+}
+
+
 /*
  * int prctl(int option, unsigned long arg2, unsigned long arg3,
  *                       unsigned long arg4, unsigned long arg5);
-- 
2.39.2

[PATCH 1/2] tools/nolibc: add pipe() support

[Zhangjin Wu]

Hi, Yuan

> pipe is crucial for shell.
>

As the syscall manpage[1] shows, pipe() is just one of the exceptions how
may require two return registers in some platforms, e.g.:
arch/mips/kernel/syscall.c

    * For historic reasons the pipe(2) syscall on MIPS has an unusual calling
     * convention.  It returns results in registers $v0 / $v1 which means there
     * is no need for it to do verify the validity of a userspace pointer
     * argument.  Historically that used to be expensive in Linux.  These days
     * the performance advantage is negligible.
     */
    asmlinkage int sysm_pipe(void)
    {
            int fd[2];
            int error = do_pipe_flags(fd, 0);
            if (error)
                    return error;
            current_pt_regs()->regs[3] = fd[1];
            return fd[0];
    }

The other exceptions are getxpid(2), getxuid(2), and getxgid(2) on
Alpha.

Since we are able to use pipe2() for pipe() (introduced from early Linux
2.6.27, glibc 2.9) and use getpid+getppid for getxpid, getuid+geteuid
for getxuid and getgid+getegit for getxgid.

So, it is possible provide such pipe() as a wraper of pipe2() and
getxpid, getxuid and getxgid as wrappers of their corresponding syscall
pairs, then, no need to provide a second return value for all of the
existing architectures, for example:

    #ifndef pipe
    int pipe(int pipefd[2])
    {
        pipe2(pipefd, 0);
    }
    #endif

And for mips:

    // may be in tools/include/nolibc/types.h ?
    struct fd_pair {
           long fd[2];
    };

    // tools/include/nolibc/arch-mips.h
    struct fd_pair pipe(void)
    {
            struct fd_pair fds;
            int pipefds[2];
    
            pipe2(pipefds, 0);
    
            fds.fd[0] = pipefds[0];
            fds.fd[1] = pipefds[1];
    
            return fds;
    }

To use such method, the test case should be changed too, perhaps an
easier method is even only provide pipe2() for all and let users define
their own pipe() if really required, we also need to change the test
case.

Best regards,
Zhangjin
[1]: https://man7.org/linux/man-pages/man2/syscall.2.html

> Signed-off-by: Yuan Tan <tanyuan@tinylab.org>
> ---
>  tools/include/nolibc/sys.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 8bfe7db20b80..09841fc266fe 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -752,6 +752,23 @@ int open(const char *path, int flags, ...)
>  }
>  
>  
> +/*
> + * int pipe(int pipefd[2]);
> + */
> +
> +static __attribute__((unused))
> +int sys_pipe(int pipefd[2])
> +{
> +	return my_syscall1(__NR_pipe, pipefd);
> +}
> +
> +static __attribute__((unused))
> +int pipe(int pipefd[2])
> +{
> +	return __sysret(sys_pipe(pipefd));
> +}
> +
> +
>  /*
>   * int prctl(int option, unsigned long arg2, unsigned long arg3,
>   *                       unsigned long arg4, unsigned long arg5);
> -- 
> 2.39.2

Re: [PATCH 1/2] tools/nolibc: add pipe() support

[Willy Tarreau]

Hi Zhangjin, hi Yuan,

On Fri, Jul 28, 2023 at 11:50:31PM +0800, Zhangjin Wu wrote:
> Hi, Yuan
> 
> > pipe is crucial for shell.
> >
> 
> As the syscall manpage[1] shows, pipe() is just one of the exceptions how
> may require two return registers in some platforms, e.g.:
> arch/mips/kernel/syscall.c
> 
>     * For historic reasons the pipe(2) syscall on MIPS has an unusual calling
>      * convention.  It returns results in registers $v0 / $v1 which means there
>      * is no need for it to do verify the validity of a userspace pointer
>      * argument.  Historically that used to be expensive in Linux.  These days
>      * the performance advantage is negligible.
>      */
(...)

Ah indeed! I vaguely remembered that I had left that one aside for some
time but did not remember why. Now I remember that I couldn't handle the
MIPS implementation by then while it used to be my primary target.

> Since we are able to use pipe2() for pipe() (introduced from early Linux
> 2.6.27, glibc 2.9) and use getpid+getppid for getxpid, getuid+geteuid
> for getxuid and getgid+getegit for getxgid.
> 
> So, it is possible provide such pipe() as a wraper of pipe2() and

Indeed.

> getxpid, getxuid and getxgid as wrappers of their corresponding syscall
> pairs,

I doubt anyone needs these ones, I didn't know them and do not even
have their man page. Let's keep the focus on what developers really use.

> then, no need to provide a second return value for all of the
> existing architectures, for example:


> 
>     #ifndef pipe
>     int pipe(int pipefd[2])
>     {
>         pipe2(pipefd, 0);
>     }
>     #endif
> 
> And for mips:
> 
>     // may be in tools/include/nolibc/types.h ?
>     struct fd_pair {
>            long fd[2];
>     };
> 
>     // tools/include/nolibc/arch-mips.h
>     struct fd_pair pipe(void)
>     {
>             struct fd_pair fds;
>             int pipefds[2];
>     
>             pipe2(pipefds, 0);
>     
>             fds.fd[0] = pipefds[0];
>             fds.fd[1] = pipefds[1];
>     
>             return fds;
>     }

This one does not have the correct prototype for the function exposed
to the user, pipe really is "int pipe(int pipefd[2])". Maybe you were
thinking about sys_pipe() instead ? But since MIPS also has pipe2() now,
there's no reason to make an exception.

> To use such method, the test case should be changed too, perhaps an
> easier method is even only provide pipe2() for all and let users define
> their own pipe() if really required, we also need to change the test
> case.

No, we need to provide users with what they need to compile standard
code. If we rely on pipe2() to deliver pipe(), that's fine. We can even
do it per-arch if there are constraints but it seems to me that pipe2()
is OK.

Thanks,
Willy

Re: [PATCH 1/2] tools/nolibc: add pipe() support

[Zhangjin Wu]

> Hi Zhangjin, hi Yuan,
> 
> On Fri, Jul 28, 2023 at 11:50:31PM +0800, Zhangjin Wu wrote:
> > Hi, Yuan
> > 
> > > pipe is crucial for shell.
> > >
> > 
> > As the syscall manpage[1] shows, pipe() is just one of the exceptions how
> > may require two return registers in some platforms, e.g.:
> > arch/mips/kernel/syscall.c
> > 
> >     * For historic reasons the pipe(2) syscall on MIPS has an unusual calling
> >      * convention.  It returns results in registers $v0 / $v1 which means there
> >      * is no need for it to do verify the validity of a userspace pointer
> >      * argument.  Historically that used to be expensive in Linux.  These days
> >      * the performance advantage is negligible.
> >      */
> (...)
> 
> Ah indeed! I vaguely remembered that I had left that one aside for some
> time but did not remember why. Now I remember that I couldn't handle the
> MIPS implementation by then while it used to be my primary target.
>

Seems pipe() is the **only** one some architectures (except Alpha)
return two values, and now we have pipe2(), so, it is ok for us to
simply use pipe2() instead ;-)

> > Since we are able to use pipe2() for pipe() (introduced from early Linux
> > 2.6.27, glibc 2.9) and use getpid+getppid for getxpid, getuid+geteuid
> > for getxuid and getgid+getegit for getxgid.
> > 
> > So, it is possible provide such pipe() as a wraper of pipe2() and
> 
> Indeed.
> 
> > getxpid, getxuid and getxgid as wrappers of their corresponding syscall
> > pairs,
> 
> I doubt anyone needs these ones, I didn't know them and do not even
> have their man page. Let's keep the focus on what developers really use.
>

Yeah.

> > then, no need to provide a second return value for all of the
> > existing architectures, for example:
> 
> 
> > 
> >     #ifndef pipe
> >     int pipe(int pipefd[2])
> >     {
> >         pipe2(pipefd, 0);
> >     }
> >     #endif
> > 
> > And for mips:
> > 
> >     // may be in tools/include/nolibc/types.h ?
> >     struct fd_pair {
> >            long fd[2];
> >     };
> > 
> >     // tools/include/nolibc/arch-mips.h
> >     struct fd_pair pipe(void)
> >     {
[...]
> >     
> >             pipe2(pipefds, 0);
[...]
> >     }
> 
> This one does not have the correct prototype for the function exposed
> to the user, pipe really is "int pipe(int pipefd[2])". Maybe you were
> thinking about sys_pipe() instead ? But since MIPS also has pipe2() now,
> there's no reason to make an exception.
>

Yes, pipe2() should be a better choice, but I have seen this sentence in
syscall manpage [1]:

       /* On Alpha, IA-64, MIPS, SuperH, and SPARC/SPARC64, pipe() has the
          following prototype; see NOTES */

       #include <unistd.h>

       struct fd_pair {
           long fd[2];
       };
       struct fd_pair pipe(void);

If it is about syscall, then we are ok to align all of the architectures
together to use "int pipe(int pipefd[2])", otherwise, it will be
required to define them in their own arch-<ARCH>.h, just like some
defined for arch-s390.h.

[1]: https://www.man7.org/linux/man-pages/man2/pipe.2.html

> > To use such method, the test case should be changed too, perhaps an
> > easier method is even only provide pipe2() for all and let users define
> > their own pipe() if really required, we also need to change the test
> > case.
> 
> No, we need to provide users with what they need to compile standard
> code. If we rely on pipe2() to deliver pipe(), that's fine. We can even
> do it per-arch if there are constraints but it seems to me that pipe2()
> is OK.
>

Ok.

Thanks,
Zhangjin

> Thanks,
> Willy

Re: [PATCH 1/2] tools/nolibc: add pipe() support

[Willy Tarreau]

On Sat, Jul 29, 2023 at 04:37:00PM +0800, Zhangjin Wu wrote:
> > This one does not have the correct prototype for the function exposed
> > to the user, pipe really is "int pipe(int pipefd[2])". Maybe you were
> > thinking about sys_pipe() instead ? But since MIPS also has pipe2() now,
> > there's no reason to make an exception.
> >
> 
> Yes, pipe2() should be a better choice, but I have seen this sentence in
> syscall manpage [1]:
> 
>        /* On Alpha, IA-64, MIPS, SuperH, and SPARC/SPARC64, pipe() has the
>           following prototype; see NOTES */
> 
>        #include <unistd.h>
> 
>        struct fd_pair {
>            long fd[2];
>        };
>        struct fd_pair pipe(void);
> 
> If it is about syscall, then we are ok to align all of the architectures
> together to use "int pipe(int pipefd[2])"

Yes it's OK, that's how applications expect it to be used:

  https://pubs.opengroup.org/onlinepubs/9699919799/functions/pipe.html

For the archs you mention above, it's the libc that wraps the call,
exactly what we ought to do as well (using pipe2() since it will be
easier).

Willy

[PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Yuan Tan]

Add a testcase of pipe that child process sends message to parent process.

Signed-off-by: Yuan Tan <tanyuan@tinylab.org>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 03b1d30f5507..43ba2884fd1e 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -767,6 +767,39 @@ int test_mmap_munmap(void)
 	return ret;
 }
 
+int test_pipe(void)
+{
+	int pipefd[2];
+	char buf[32];
+	pid_t pid;
+	char *msg = "hello, nolibc";
+
+	if (pipe(pipefd) == -1)
+		return 1;
+
+	pid = fork();
+
+	switch (pid) {
+	case -1:
+		return 1;
+
+	case 0:
+		close(pipefd[0]);
+		write(pipefd[1], msg, strlen(msg));
+		close(pipefd[1]);
+		exit(EXIT_SUCCESS);
+
+	default:
+		close(pipefd[1]);
+		read(pipefd[0], buf, 32);
+		close(pipefd[0]);
+		wait(NULL);
+
+		if (strcmp(buf, msg))
+			return 1;
+		return 0;
+	}
+}
 
 /* Run syscall tests between IDs <min> and <max>.
  * Return 0 on success, non-zero on failure.
@@ -851,6 +884,7 @@ int run_syscall(int min, int max)
 		CASE_TEST(mmap_munmap_good);  EXPECT_SYSZR(1, test_mmap_munmap()); break;
 		CASE_TEST(open_tty);          EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break;
 		CASE_TEST(open_blah);         EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break;
+		CASE_TEST(pipe);              EXPECT_SYSZR(1, test_pipe()); break;
 		CASE_TEST(poll_null);         EXPECT_SYSZR(1, poll(NULL, 0, 0)); break;
 		CASE_TEST(poll_stdout);       EXPECT_SYSNE(1, ({ struct pollfd fds = { 1, POLLOUT, 0}; poll(&fds, 1, 0); }), -1); break;
 		CASE_TEST(poll_fault);        EXPECT_SYSER(1, poll((void *)1, 1, 0), -1, EFAULT); break;
-- 
2.39.2

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Thomas Weißschuh]

On 2023-07-25 14:01:30-0400, Yuan Tan wrote:
> Add a testcase of pipe that child process sends message to parent process.
> 
> Signed-off-by: Yuan Tan <tanyuan@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 34 ++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 03b1d30f5507..43ba2884fd1e 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -767,6 +767,39 @@ int test_mmap_munmap(void)
>  	return ret;
>  }
>  
> +int test_pipe(void)
> +{
> +	int pipefd[2];
> +	char buf[32];
> +	pid_t pid;
> +	char *msg = "hello, nolibc";

const char * const

> +
> +	if (pipe(pipefd) == -1)
> +		return 1;
> +
> +	pid = fork();
> +
> +	switch (pid) {
> +	case -1:
> +		return 1;
> +
> +	case 0:
> +		close(pipefd[0]);
> +		write(pipefd[1], msg, strlen(msg));

Isn't this missing to write trailing the 0 byte?
Also check the return value.

> +		close(pipefd[1]);

Do we need to close the pipefds? The process is exiting anyways.

> +		exit(EXIT_SUCCESS);
> +
> +	default:
> +		close(pipefd[1]);
> +		read(pipefd[0], buf, 32);

Use sizeof(buf). Check return value == strlen(msg).

> +		close(pipefd[0]);
> +		wait(NULL);

waitpid(pid, NULL, 0);

> +
> +		if (strcmp(buf, msg))
> +			return 1;
> +		return 0;

return !!strcmp(buf, msg);

> +	}
> +}
>  
>  /* Run syscall tests between IDs <min> and <max>.
>   * Return 0 on success, non-zero on failure.
> @@ -851,6 +884,7 @@ int run_syscall(int min, int max)
>  		CASE_TEST(mmap_munmap_good);  EXPECT_SYSZR(1, test_mmap_munmap()); break;
>  		CASE_TEST(open_tty);          EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break;
>  		CASE_TEST(open_blah);         EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break;
> +		CASE_TEST(pipe);              EXPECT_SYSZR(1, test_pipe()); break;
>  		CASE_TEST(poll_null);         EXPECT_SYSZR(1, poll(NULL, 0, 0)); break;
>  		CASE_TEST(poll_stdout);       EXPECT_SYSNE(1, ({ struct pollfd fds = { 1, POLLOUT, 0}; poll(&fds, 1, 0); }), -1); break;
>  		CASE_TEST(poll_fault);        EXPECT_SYSER(1, poll((void *)1, 1, 0), -1, EFAULT); break;
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Willy Tarreau]

On Sun, Jul 30, 2023 at 12:17:24AM +0200, Thomas Weißschuh wrote:
> > +	case 0:
> > +		close(pipefd[0]);
> > +		write(pipefd[1], msg, strlen(msg));
> 
> Isn't this missing to write trailing the 0 byte?

It depends if the other side expects to get the trailing 0.
In general it's better to avoid sending it since it's only
used for internal representation, and the other side must
be prepared to receive anything anyway.

> Also check the return value.

Indeed!

> > +		close(pipefd[1]);
> 
> Do we need to close the pipefds? The process is exiting anyways.

It's better to, because we could imagine looping over the tests for
example. Thus each test shoulld have as little impact as possible
on other tests.

> > +		exit(EXIT_SUCCESS);
> > +
> > +	default:
> > +		close(pipefd[1]);
> > +		read(pipefd[0], buf, 32);
> 
> Use sizeof(buf). Check return value == strlen(msg).
> 
> > +		close(pipefd[0]);
> > +		wait(NULL);
> 
> waitpid(pid, NULL, 0);
> 
> > +
> > +		if (strcmp(buf, msg))
> > +			return 1;
> > +		return 0;
> 
> return !!strcmp(buf, msg);

In fact before that we need to terminate the output buffer. If for any
reason the transfer fails (e.g. the syscall fails or transfers data at
another location or of another length, we could end up comparing past
the end of the buffer. Thus I suggest adding this immediately after the
read():

		buf[sizeof(buf) - 1] = 0;

Willy

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Thomas Weißschuh]

On 2023-07-30 05:33:43+0200, Willy Tarreau wrote:
> On Sun, Jul 30, 2023 at 12:17:24AM +0200, Thomas Weißschuh wrote:
> > > +	case 0:
> > > +		close(pipefd[0]);
> > > +		write(pipefd[1], msg, strlen(msg));
> > 
> > Isn't this missing to write trailing the 0 byte?
> 
> It depends if the other side expects to get the trailing 0.
> In general it's better to avoid sending it since it's only
> used for internal representation, and the other side must
> be prepared to receive anything anyway.
> 
> > Also check the return value.
> 
> Indeed!
> 
> > > +		close(pipefd[1]);
> > 
> > Do we need to close the pipefds? The process is exiting anyways.
> 
> It's better to, because we could imagine looping over the tests for
> example. Thus each test shoulld have as little impact as possible
> on other tests.

I meant the newly forked child exiting, not nolibc-test in general.
The exit is just below, so the fds in the child are close here anyways.
                |
		|
		v
> > > +		exit(EXIT_SUCCESS);
> > > +
> > > +	default:
> > > +		close(pipefd[1]);
> > > +		read(pipefd[0], buf, 32);
> > 
> > Use sizeof(buf). Check return value == strlen(msg).
> > 
> > > +		close(pipefd[0]);
> > > +		wait(NULL);
> > 
> > waitpid(pid, NULL, 0);
> > 
> > > +
> > > +		if (strcmp(buf, msg))
> > > +			return 1;
> > > +		return 0;
> > 
> > return !!strcmp(buf, msg);
> 
> In fact before that we need to terminate the output buffer. If for any
> reason the transfer fails (e.g. the syscall fails or transfers data at
> another location or of another length, we could end up comparing past
> the end of the buffer. Thus I suggest adding this immediately after the
> read():
> 
> 		buf[sizeof(buf) - 1] = 0;

This would still access uninitialized memory and lead to UB in strcmp as
not all bytes in buf were written to by read().

If we want to be really sure we should use memcmp() instead of strcmp().
For memcmp() I would prefer to transfer and check without the '\0', so
my review comments from before need to be adapted a bit.

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Willy Tarreau]

On Sun, Jul 30, 2023 at 08:55:47AM +0200, Thomas Weißschuh wrote:
> On 2023-07-30 05:33:43+0200, Willy Tarreau wrote:
> > On Sun, Jul 30, 2023 at 12:17:24AM +0200, Thomas Weißschuh wrote:
> > > > +	case 0:
> > > > +		close(pipefd[0]);
> > > > +		write(pipefd[1], msg, strlen(msg));
> > > 
> > > Isn't this missing to write trailing the 0 byte?
> > 
> > It depends if the other side expects to get the trailing 0.
> > In general it's better to avoid sending it since it's only
> > used for internal representation, and the other side must
> > be prepared to receive anything anyway.
> > 
> > > Also check the return value.
> > 
> > Indeed!
> > 
> > > > +		close(pipefd[1]);
> > > 
> > > Do we need to close the pipefds? The process is exiting anyways.
> > 
> > It's better to, because we could imagine looping over the tests for
> > example. Thus each test shoulld have as little impact as possible
> > on other tests.
> 
> I meant the newly forked child exiting, not nolibc-test in general.
> The exit is just below, so the fds in the child are close here anyways.

Ah OK, but still it remains cleaner with it IMHO (i.e. better rely on
explicit things in tests, that's less doubts when they fail).

> > > > +	default:
> > > > +		close(pipefd[1]);
> > > > +		read(pipefd[0], buf, 32);
> > > 
> > > Use sizeof(buf). Check return value == strlen(msg).
> > > 
> > > > +		close(pipefd[0]);
> > > > +		wait(NULL);
> > > 
> > > waitpid(pid, NULL, 0);
> > > 
> > > > +
> > > > +		if (strcmp(buf, msg))
> > > > +			return 1;
> > > > +		return 0;
> > > 
> > > return !!strcmp(buf, msg);
> > 
> > In fact before that we need to terminate the output buffer. If for any
> > reason the transfer fails (e.g. the syscall fails or transfers data at
> > another location or of another length, we could end up comparing past
> > the end of the buffer. Thus I suggest adding this immediately after the
> > read():
> > 
> > 		buf[sizeof(buf) - 1] = 0;
> 
> This would still access uninitialized memory and lead to UB in strcmp as
> not all bytes in buf were written to by read().
> 
> If we want to be really sure we should use memcmp() instead of strcmp().
> For memcmp() I would prefer to transfer and check without the '\0', so
> my review comments from before need to be adapted a bit.

In fact you make a good point regarding the fact that the test doesn't
use read()'s return value. This problem totally goes away if the return
value is used, e.g.:

      len = read(pipefd[0], buf, sizeof(buf));
      close(pipefd[0]);
      waitpid(pid, NULL, 0);
      return len < 0 || len > sizeof(buf) || len > strlen(msg) || memcmp(buf, msg, len) != 0;

Willy

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Thomas Weißschuh]

On 2023-07-30 09:12:27+0200, Willy Tarreau wrote:
> On Sun, Jul 30, 2023 at 08:55:47AM +0200, Thomas Weißschuh wrote:
> > On 2023-07-30 05:33:43+0200, Willy Tarreau wrote:
> > > On Sun, Jul 30, 2023 at 12:17:24AM +0200, Thomas Weißschuh wrote:
> > > > > +	case 0:
> > > > > +		close(pipefd[0]);
> > > > > +		write(pipefd[1], msg, strlen(msg));
> > > > 
> > > > Isn't this missing to write trailing the 0 byte?
> > > 
> > > It depends if the other side expects to get the trailing 0.
> > > In general it's better to avoid sending it since it's only
> > > used for internal representation, and the other side must
> > > be prepared to receive anything anyway.
> > > 
> > > > Also check the return value.
> > > 
> > > Indeed!
> > > 
> > > > > +		close(pipefd[1]);
> > > > 
> > > > Do we need to close the pipefds? The process is exiting anyways.
> > > 
> > > It's better to, because we could imagine looping over the tests for
> > > example. Thus each test shoulld have as little impact as possible
> > > on other tests.
> > 
> > I meant the newly forked child exiting, not nolibc-test in general.
> > The exit is just below, so the fds in the child are close here anyways.
> 
> Ah OK, but still it remains cleaner with it IMHO (i.e. better rely on
> explicit things in tests, that's less doubts when they fail).

Accepted :-)

> > > > > +	default:
> > > > > +		close(pipefd[1]);
> > > > > +		read(pipefd[0], buf, 32);
> > > > 
> > > > Use sizeof(buf). Check return value == strlen(msg).
> > > > 
> > > > > +		close(pipefd[0]);
> > > > > +		wait(NULL);
> > > > 
> > > > waitpid(pid, NULL, 0);
> > > > 
> > > > > +
> > > > > +		if (strcmp(buf, msg))
> > > > > +			return 1;
> > > > > +		return 0;
> > > > 
> > > > return !!strcmp(buf, msg);
> > > 
> > > In fact before that we need to terminate the output buffer. If for any
> > > reason the transfer fails (e.g. the syscall fails or transfers data at
> > > another location or of another length, we could end up comparing past
> > > the end of the buffer. Thus I suggest adding this immediately after the
> > > read():
> > > 
> > > 		buf[sizeof(buf) - 1] = 0;
> > 
> > This would still access uninitialized memory and lead to UB in strcmp as
> > not all bytes in buf were written to by read().
> > 
> > If we want to be really sure we should use memcmp() instead of strcmp().
> > For memcmp() I would prefer to transfer and check without the '\0', so
> > my review comments from before need to be adapted a bit.
> 
> In fact you make a good point regarding the fact that the test doesn't
> use read()'s return value. This problem totally goes away if the return
> value is used, e.g.:
> 
>       len = read(pipefd[0], buf, sizeof(buf));
>       close(pipefd[0]);
>       waitpid(pid, NULL, 0);
>       return len < 0 || len > sizeof(buf) || len > strlen(msg) || memcmp(buf, msg, len) != 0;

Wouldn't this happily accept len == 0?

Why not just:

if (len != strlen(msg))
  return 1;
return !!memcmp(buf, msg, len);

Also so far we have assumed that one call one call to read() is enough.
But looking at pipe(7) this is not guaranteed by the spec.
If we want to be really sure, a loop around read() seems to be necessary.

Re: [PATCH 2/2] selftests/nolibc: add testcase for pipe.

[Willy Tarreau]

On Sun, Jul 30, 2023 at 10:07:24AM +0200, Thomas Weißschuh wrote:
> > In fact you make a good point regarding the fact that the test doesn't
> > use read()'s return value. This problem totally goes away if the return
> > value is used, e.g.:
> > 
> >       len = read(pipefd[0], buf, sizeof(buf));
> >       close(pipefd[0]);
> >       waitpid(pid, NULL, 0);
> >       return len < 0 || len > sizeof(buf) || len > strlen(msg) || memcmp(buf, msg, len) != 0;
> 
> Wouldn't this happily accept len == 0?
> 
> Why not just:
> 
> if (len != strlen(msg))
>   return 1;
> return !!memcmp(buf, msg, len);

Indeed, works for me.

> Also so far we have assumed that one call one call to read() is enough.
> But looking at pipe(7) this is not guaranteed by the spec.
> If we want to be really sure, a loop around read() seems to be necessary.

In practice it will be OK as the message is small and sent in one syscall,
so let's not care too much about this for now.

Willy