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

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

On Tue, 16 May 2023 at 13:26, Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> On 16/05/23 08:54, Luca Boccassi wrote:
> > 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?
>
> Because -1 will be parsed as ULONG_MAX.  For instance, with the inputs:
>
> Input:             | Function result |         parse_fdinfo_t
> -------------------|-----------------|-----------------------
> "Pid: 0"           |               1 |        {1,          0}
> "Pid: 1"           |               1 |        {1,          1}
> "Pid: 2147483647"  |               1 |        {1, 2147483647}
> "Pid: 2147483648"  |               0 |        {0,         -1}
> "Pid: -1"          |               1 |        {1,         -1}
> "Pid: -3"          |               0 |        {0,         -1}
> "Pid: -24x"        |               0 |        {0,         -1}
>
> So only if the PID if positive less than INT_MAX or -1 the function
> will set that the PID as found.

This seems excessively convoluted and fragile to me, relying on
overflows and so on. I cannot stress this enough, in order to use this
from systemd et al I need to be absolutely certain that garbage input
is not treated the same as ESCHR. Why not just use strtoll or so, to
get an exact result out of it?

Kind regards,
Luca Boccassi