Re: [PATCH v3 3/3] linux: Add pidfd_getpid

[Luca Boccassi <luca.boccassi@gmail.com>]

On Tue, 16 May 2023 at 12:46, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> This interface allows to obtain the associated pid ID from the
> process file descriptor.  It is done by parsing the procps fdinfo
> information.  Its prototype is:
>
>    pid_t pidfd_getpid (int fd)
>
> It returns the associated pid or -1 in case of an error and set the
> errno accordingly.  The possible errno values are the smae from
> open, read, and close (used on procps parsing), along with:
>
>    - EINVAL if the FP is negative (similar to fexecve).
>
>    - EBADF if the FD does not have a PID associated of if the fdinfo
>      fields contains a value larger than pid_t.
>
>    - EREMOTE if the PID is in a separate namespace.
>
>    - ESRCH if the process is already terminated.
>
> Checked on x86_64-linux-gnu on Linux 4.15 (no CLONE_PID or waitid
> support), Linux 5.15 (only clone support), and Linux 5.19 (full
> support including clone3).
> ---
<..>
> +#define FDINFO_TO_FILENAME_PREFIX "/proc/self/fdinfo/"
> +
> +#define FDINFO_FILENAME_LEN \
> +  (sizeof (FDINFO_TO_FILENAME_PREFIX) + INT_STRLEN_BOUND (int))
> +
> +struct parse_fdinfo_t
> +{
> +  bool found;
> +  pid_t pid;
> +};
> +
> +static int
> +parse_fdinfo (const char *l, void *arg)
> +{
> +  enum { fieldlen = sizeof ("Pid:") - 1 };
> +  if (strncmp (l, "Pid:", fieldlen) != 0)
> +    return 0;
> +
> +  l += fieldlen;
> +
> +  char *endp;
> +  unsigned long n = strtoul (l, &endp, 10);
> +  if (l == endp || (n > INT_MAX && n != ULONG_MAX))
> +    return 0;

How can this tell the difference between '-1' and garbage input? It
seems to me this will confuse mangled input here with ESRCH, given the
pid in fdinfo is initialized to -1, no?

> +  struct parse_fdinfo_t *fdinfo = arg;
> +  fdinfo->found = true;
> +  fdinfo->pid = n;
> +
> +  return 1;
> +}
> +
> +pid_t
> +pidfd_getpid (int fd)
> +{
> +  if (__glibc_unlikely (fd < 0))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +
> +  char fdinfoname[FDINFO_FILENAME_LEN];
> +
> +  char *p = mempcpy (fdinfoname, FDINFO_TO_FILENAME_PREFIX,
> +                    strlen (FDINFO_TO_FILENAME_PREFIX));
> +  *_fitoa_word (fd, p, 10, 0) = '\0';
> +
> +  struct parse_fdinfo_t fdinfo = { .found = false, .pid = -1 };
> +  if (procutils_read_file (fdinfoname, parse_fdinfo, &fdinfo) == -1)
> +    /* The fdinfo contains an invalid 'Pid:' value.  */
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EBADF);
> +
> +  /* The FD does not have a 'Pid:' entry associated.  */
> +  if (!fdinfo.found)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EBADF);
> +
> +  /* The pidfd cannot be resolved because it is in a separate pid
> +     namespace.  */
> +  if (fdinfo.pid == 0)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EREMOTE);
> +
> +  /* A negative value means the process is terminated.  */
> +  if (fdinfo.pid < 0)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ESRCH);

Here it just assumes this is an ESRSCH case, but again it could be
failing to parse it for other corner cases of stroul. It should return
ESRCH _only_ if it really parsed -1 from fdinfo, otherwise we cannot
rely on it.

Kind regards,
Luca Boccassi