[PATCH] stdlib: Perform NULL pointer check for the getenv argument

[Carlos O'Donell]

On 6/7/23 14:04, Dennis Brendel via Libc-alpha wrote:
> 'name' is just de-referenced without checking for it being non-NULL.

The input must be non-NULL.

The headers are marked up with '__nonnull ((1))' specifically to catch this issue
(along with other markup).

To put it another way, the "conditions of use" for that API require the argument
is non-NULL.

> Passing NULL is not something one should do in the first place, but	
> returning NULL seems to be reasonable in that case instead of just
> waiting for the segfault that might or might not be handled.
> 
> This adds another barrier for e.g. safety applications.

Where there is no API contract to check for NULL we should not check for NULL.

We should dereference as quickly as we can and crash so the misuse of the API
can be detected quickly during development.

Returning NULL where the interface has no agreement to do so does the 
*opposite* of improving safety because it makes it difficult to track down
the original error when an improperly used interface hides the
issue by returning a non-error return e.g. NULL.

Please see the discussion in "Style and Conventions" for Error Handling:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Error_Handling

Also __environ being NULL is not a valid scenario that should arise, when the
previous process called exec it should have passed a non-NULL environ, so one
could argue we could drop that check for conforming applications too, but it's
possible that this has been there for long enough that through Hyrum's law
we have applications depending on this.

There was a recent discussion about this in musl:
https://www.openwall.com/lists/musl/2023/06/09/1

> ---
>  stdlib/getenv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/stdlib/getenv.c b/stdlib/getenv.c
> index 8408e641a6..794f3e00d3 100644
> --- a/stdlib/getenv.c
> +++ b/stdlib/getenv.c
> @@ -22,7 +22,7 @@
>  char *
>  getenv (const char *name)
>  {
> -  if (__environ == NULL || name[0] == '\0')
> +  if (__environ == NULL || name == NULL || name[0] == '\0')
>      return NULL;
>  
>    size_t len = strlen (name);

-- 
Cheers,
Carlos.