Re: [PATCH] libgomp: Robustify omp_get_device_distances: ... some more [PR125877]

[Tobias Burnus]

Thomas Schwinge wrote:

       FILE *f = fopen ("/sys/devices/system/node/online", "r");
       if (f == NULL)
         goto fail;
       char *lineptr = NULL;
     [...]
     fail:
       free (lineptr);

That happens when changing when happens during cleanup and
changing how many cleanup goto targets are.

Well spotted!

       char *lineptr = NULL;
       size_t nline;
       if (getline (&lineptr, &nline, f) <= 0)
     [...]

The 'getline' API states:

|  -- Function: ssize_t getline (char **LINEPTR, size_t *N, FILE *STREAM)
| [...]
|      If you set ‘*LINEPTR’ to a null pointer, and ‘*N’ to zero, before
|      the call, then ‘getline’ allocates the initial buffer [...]

..., so we should initialize 'nline' to zero.

This does not match the specification:

"The application shall ensure that *lineptr is a valid argument that
 could be passed to the free() function. If *n is non-zero, the
 application shall ensure that *lineptr either points to an object
 of size at least *n bytes, or is a null pointer.

 If *lineptr is a null pointer or if the object pointed to by *lineptr
 is of insufficient size, an object shall be allocated as if by malloc()"

Thus, if lineptr == NULL, the value of 'n' (alias 'nline') does not
matter.

See POSIX, i.e.IEEE Std 1003.1-2024
https://pubs.opengroup.org/onlinepubs/9799919799/functions/getdelim.html

* * *

        PR libgomp/125877
        libgomp/
        * config/linux/numa.c (gomp_get_numa_distance): Move 'lineptr'
        initialization earlier, fix 'getline' call, avoid double 'free'.
---
  libgomp/config/linux/numa.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libgomp/config/linux/numa.c b/libgomp/config/linux/numa.c
index 85214e56dc41..e092f398540f 100644
--- a/libgomp/config/linux/numa.c
+++ b/libgomp/config/linux/numa.c
@@ -87,14 +87,13 @@ retry:
    /* Obtain numa nodes.  */

num = 0;

+  char *lineptr = NULL;
    FILE *f = fopen ("/sys/devices/system/node/online", "r");
    if (f == NULL)
      goto fail;
-  char *lineptr = NULL;
-  size_t nline;
+  size_t nline = 0;

I think it looks more consistent if nline's declaration is placed
next to the lineptr's one, which means also moving it before FILE.

BTW: The n = 0 is not needed not only according to POSIX but also:

GLIBC has libio/iogetdelim.c → __getdelim):

  if (*lineptr == NULL || *n == 0)
    {
      *n = 120;
      *lineptr = (char *) malloc (*n);

and Newlib has (newlib/libc/stdio/getdelim.c → __getdelim):

  if (buf == NULL || *n < MIN_LINE_SIZE)
    {
      buf = (char *)realloc (*bufptr, DEFAULT_LINE_SIZE);

Thus, as mentioned, I don't see the need for the "= 0"
(not that it really harms).

    if (getline (&lineptr, &nline, f) <= 0)
      {
-      free (lineptr);
        fclose (f);
        goto fail;
      }

Also well spotted - as mentioned, I kept adding more goto
targets + doing cleanup + goto, but at the end I consolidated
it to a single one, but obviously failed to update all cases
for it :-/

Thanks for the fix, which LGTM.

Tobias