Re: [PATCH v7 8/8] linux: Add pidfd_getpid

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> +If the operation fails, @code{pidfd_getpid} return @code{-1} and the following
> +@code{errno} error conditionas are defined:
> +
> +@table @code
> +@item EBADF
> +The input file descriptor is invalid, does not have a pidfd associated, or an
> +error has occurred parsing the kernel data.
> +@item EREMOTE
> +There is no process ID to denote the process in the current namespace.
> +@item ESRCH
> +The process for which the file descriptor refers to is terminated.
> +@end table

Maybe document ENOENT (/proc not mounted), ENFILE, EMFILE, ENOMEM as
well?

There are missing spaces in a few places:

+  while (*l == ' ' || (unsigned int)(*l) -'\t' < 5)
+      if ((unsigned int)(*l) - '0' >= 10)
+typedef int (*procutils_closure_t)(const char *line, void *arg);
+      char buf[CMSG_SPACE(sizeof(int))];

> diff --git a/sysdeps/unix/sysv/linux/pidfd_getpid.c b/sysdeps/unix/sysv/linux/pidfd_getpid.c
> new file mode 100644
> index 0000000000..46848a5983
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/pidfd_getpid.c

> +  bool neg = false;
> +  switch (*l)
> +    {
> +    case '-': neg = true;
> +    case '+': l++;

'+' should probably return -1?

> +    }
> +
> +  if (*l == '\0')
> +    return 0;
> +
> +  int n = 0;
> +  while (*l != '\0')
> +    {
> +      /* Check if '*l' is a digit.  */
> +      if ((unsigned int)(*l) - '0' >= 10)

It's a strange way to write this condition. '0' <= *l && l <= '9' should
work equally well.  I know is supposed to optimize this into one
condition, but it's not immediately obvious why this works with an early
cast of unsigned int instead of unsigned char.

> +      /* Ignore invalid large values.  */
> +      if (INT_MULTIPLY_WRAPV (10, n, &n)
> +          || INT_ADD_WRAPV (n, *l++ - '0', &n))
> +        return 0;

Shouldn't this return -1?

I think these error returns should set errno (EINVAL perhaps, since
that's unlikely to come from read or open), so that we have a chance to
identify parser problems.


> diff --git a/sysdeps/unix/sysv/linux/procutils.c b/sysdeps/unix/sysv/linux/procutils.c
> new file mode 100644
> index 0000000000..83b327cb9a
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/procutils.c
> @@ -0,0 +1,104 @@
> +/* Utilities functions to read/parse Linux procfs and sysfs.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <assert.h>
> +#include <not-cancel.h>
> +#include <procutils.h>
> +#include <string.h>
> +
> +static int
> +next_line (char **r, int fd, char *const buffer, char **cp, char **re,
> +           char *const buffer_end)
> +{
> +  char *res = *cp;
> +  char *nl = memchr (*cp, '\n', *re - *cp);
> +  if (nl == NULL)
> +    {
> +      if (*cp != buffer)
> +        {
> +          if (*re == buffer_end)
> +            {
> +              memmove (buffer, *cp, *re - *cp);
> +              *re = buffer + (*re - *cp);
> +              *cp = buffer;
> +
> +              ssize_t n = __read_nocancel (fd, *re, buffer_end - *re);

Missing TEMP_FAILURE_RETRY, I would (also below, and further below for
__open64_nocancel).

> +              if (n < 0)
> +                return -1;
> +
> +              *re += n;
> +
> +              nl = memchr (*cp, '\n', *re - *cp);
> +              while (nl == NULL && *re == buffer_end)
> +                {
> +                  /* Truncate too long lines.  */
> +                  *re = buffer + 3 * (buffer_end - buffer) / 4;
> +                  n = __read_nocancel (fd, *re, buffer_end - *re);
> +                  if (n < 0)
> +                    return -1;
> +
> +                  nl = memchr (*re, '\n', n);
> +                  **re = '\0';
> +                  *re += n;
> +                }

Should we just skip long lines?  The 3/4 business is a bit strange.

This results in an endless loop if the file does not end with '\n', I
think.

> diff --git a/sysdeps/unix/sysv/linux/procutils.h b/sysdeps/unix/sysv/linux/procutils.h
> new file mode 100644
> index 0000000000..64e1080920
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/procutils.h
> @@ -0,0 +1,35 @@

> +typedef int (*procutils_closure_t)(const char *line, void *arg);
> +
> +/* Open and read the path FILENAME, line per line, and call CLOSURE with
> +   argument ARG on each line.  The read is done with a static buffer,
> +   with non-cancellable calls, and the line is null terminated.
> +
> +   The CLOSURE should return true if the read should continue, or false
> +   if the function should stop.
> +
> +   It returns 0 in case of success, or -1 otherwise.  */
> +int procutils_read_file (const char *filename, procutils_closure_t closure,
> +			 void *arg) attribute_hidden;
> +

A comment should say whether '\n' is included in line argument, and what
happens to overlong lines.

The return value for the closure should be an actual bool, or otherwise
the int return value should be passed through to the caller of
procutils_read_file.

> diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c b/sysdeps/unix/sysv/linux/tst-pidfd.c
> index 64d8a2ef40..53d223f702 100644
> --- a/sysdeps/unix/sysv/linux/tst-pidfd.c
> +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c
> @@ -18,6 +18,7 @@

> +  /* Check if pidfd_getpid returns ESRCH for exited subprocess.  */
> +  {
> +    int pidfd;
> +    pid_t pidfork = pidfd_fork (&pidfd, -1, 0);
> +    if (pidfork == 0)
> +      _exit (EXIT_SUCCESS);
> +
> +    /* The process might be still running or already in zombie state, in any
> +       case the PID is still allocated to the process.  */
> +    pid_t pid = pidfd_getpid (pidfd);
> +    if (pid > 0)
> +      support_process_state_wait (pid, support_process_state_zombie);

The condition does not match the comment.  I don't know which one is
correct.  Please verify that pid > 0 (if the PID remains available), or
change the comment to “in [either] case”.

> diff --git a/sysdeps/unix/sysv/linux/tst-pidfd_getpid.c b/sysdeps/unix/sysv/linux/tst-pidfd_getpid.c
> new file mode 100644
> index 0000000000..41d03a04ad
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-pidfd_getpid.c

> +  TEST_VERIFY_EXIT (socketpair (AF_UNIX, SOCK_STREAM, 0, sockfd) == 0);
> +
> +  /* Check if pidfd_getpid returns EREMOTE for process not in current
> +     namespace.  */
> +  {
> +    int pidfd;
> +    pid_t pid = pidfd_fork (&pidfd, -1, 0);

I think you can avoid the file descriptor passing if you call pidfd_fork
to create an unrelated descriptor, and then do the namespace thing below
after another fork.  This way, the descriptor will just be inherited via
fork.

> +    send_fd (sockfd[1], pidfd);
> +
> +    siginfo_t info;
> +    TEST_COMPARE (waitid (P_PIDFD, pidfd, &info, WEXITED), 0);
> +    if (info.si_status == EXIT_UNSUPPORTED)
> +      FAIL_UNSUPPORTED ("unable to unshare user/fs/pid");
> +    TEST_COMPARE (info.si_status, 0);
> +    TEST_COMPARE (info.si_code, CLD_EXITED);

I think this could have a few tests, like the pidfd_getpid value
matching what comes back subsequently in si_pid.

> diff --git a/sysdeps/unix/sysv/linux/sys/pidfd.h b/sysdeps/unix/sysv/linux/sys/pidfd.h
> index 87095212a7..8cf4df6b81 100644
> --- a/sysdeps/unix/sysv/linux/sys/pidfd.h
> +++ b/sysdeps/unix/sysv/linux/sys/pidfd.h
> @@ -67,4 +67,8 @@ extern int pidfd_send_signal (int __pidfd, int __sig, siginfo_t *__info,
>  extern pid_t pidfd_fork (int *__pidfd, int __cgroup, unsigned int __flags)
>    __THROW;
>  
> +/* Query the process ID (PID) from process descriptor __FD.  Return the PID
> +   or -1 in case of an error.  */
> +extern pid_t pidfd_getpid (int __fd) __THROW;
> +

__FD should be FD.


Thanks,
Florian