stdlib: Perform NULL pointer check for the getenv argument

  'name' is just de-referenced without checking for it being 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.
---
 stdlib/getenv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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);
  

Dennis Brendel via Libc-alpha <libc-alpha@sourceware.org> writes:

> 'name' is just de-referenced without checking for it being 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.
>

See Carlos' email for why we can't do this, but I'm wondering what the
motivation here was and if we can do something to help with that?
  

On 6/12/23 23:22, Carlos O'Donell wrote:
> 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).

Uh, I completely missed that.

In discussions I had with others setting the nonull attribute was deemed a valid alternative choice, so apologies for overlooking this.

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

Yeah, I agree.

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

That makes sense to me, thanks for sharing!

> 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

That's interesting, I have yet to learn lots on that front.

Thank you for taking the time and explaining, Carlos, I really appreciate your help and hope to be able soon to make proper contributions :-)


>> ---
>>  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);
>
  

On 6/12/23 23:30, Sam James wrote:
> 
> Dennis Brendel via Libc-alpha <libc-alpha@sourceware.org> writes:
> 
>> 'name' is just de-referenced without checking for it being 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.
>>
> 
> See Carlos' email for why we can't do this, but I'm wondering what the
> motivation here was and if we can do something to help with that?

Maybe to sum it up as some overambitious activity of myself is not too
far off :-D

The motivation is to make using glibc as "safe" as possible upstream as
part of Red Hat's In-Vehicle Operating System effort.

In this specific case I think we are good, since the function prototype,
as Carlos pointed out, already specifies the use of getenv() and has the
corresponding function attribute set.