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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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.

> 
>> +  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.

It is already handled by the 'found' check above, where for values not in 
PID range or error in strtoul, EBADF will be returned.  I might improve
the comments to make it clear.