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

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

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.

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.