Make sure to never allocate zero-length memory buffer in opal_argv_join_range

[devreal]

This silences a warning issued by GCC 10.2.0, warning about str[--str_len] = '\0' writing out of bounds if str_len is 0:

../../../opal/util/argv.c: In function ‘opal_argv_join_range’:
../../../opal/util/argv.c:368:20: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  368 |     str[--str_len] = '\0';
      |     ~~~~~~~~~~~~~~~^~~~~~
../../../opal/util/argv.c:363:32: note: at offset -1 to an object with size 0 allocated by ‘malloc’ here
  363 |     if (NULL == (str = (char*) malloc(str_len)))
      |                                ^~~~~~~~~~~~~~~

This also ensures that 0 is not passed to malloc, which may return NULL and lead to a unintended return of NULL instead of an empty string.

The change to greater-or-equal in (int)start >= opal_argv_count(argv) is meant to catch the case of argv[start] == NULL early.

Signed-off-by: Joseph Schuchart [email protected]

[bosilca]

We need to be consistent in this function when something didn't go as planned: either we return strdup("") or NULL, but consistently.

[devreal]

I think the point of returning NULL is the assumption that if malloc returns NULL we're out-of-memory, so strdup won't work either.

[jsquyres]

Good catch!

...that being said, do we use opal_argv_join_range() anywhere in the code base? This feels very much like a "Jeff tried to be complete" kind of function...

After some grep'ing, it doesn't look like we use this function anywhere. Would a better solution be just to remove this function altogether?

[devreal]

Sure, I'm happy to remove it altogether. It won't hurt to keep it in case someone somewhere at some point wants that functionality though ^^

[jsquyres]

Sure, I'm happy to remove it altogether. It won't hurt to keep it in case someone somewhere at some point wants that functionality though ^^

Your call. Looks like it's used in PMIx and we used to use it in the rsh PML (which is now PRTE). I.e., we don't use it in Open MPI anywhere.

[devreal]

I'd say we leave it in, it might be handy for debugging purposes and there is no harm in having it.

[devreal]

I believe I have seen this warning with PRRTE and PMIx, too @rhc54

[rhc54]

Thanks - I have been tracking this PPR and plan to backport it to both of those. Appreciate you ensuring I saw it!