[PATCH 2/2] shared.c: Only setenv() if value is non-null

[Ferry Huberts]

On 07/26/2011 04:48 PM, Lukas Fleischer wrote:
> On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
>> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
>>> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
>>>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
>>>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
>>>>> segfault if we pass a NULL value. Add an additional check to avoid this.
>>>>>
>>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>>>> ---
>>>>>  shared.c |    2 +-
>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/shared.c b/shared.c
>>>>> index 75c4b5c..0c8ce3e 100644
>>>>> --- a/shared.c
>>>>> +++ b/shared.c
>>>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>>>>>  	p = env_vars;
>>>>>  	q = p + env_var_count;
>>>>>  	for (; p < q; p++)
>>>>> -		if (setenv(p->name, p->value, 1))
>>>>> +		if (p->value && setenv(p->name, p->value, 1))
>>>>>  			fprintf(stderr, warn, p->name, p->value);
>>>>>  }
>>>>>  
>>>>
>>>> Lukas,
>>>>
>>>> I don't agree with this patch.
>>>> >From cgitrc.5.txt, lines 501-503:
>>>>
>>>>> If a setting is not defined for a repository and the corresponding global
>>>>> setting is also not defined (if applicable), then the corresponding
>>>>> environment variable will be an empty string.
>>>>
>>>> So I'd like this differently, an empty string must be set.
>>>
>>> -1. Is there any reason to do this? Imho, we should leave undefined
>>> variables unset and fix the documentation here. This is way more
>>> straightforward and allows for distinguishing between unset and empty
>>> values (this is cleaner, even though there might not be a use case yet).
>>
>> Not true, I run this code on my servers.
>>
>> I agree with the fact that it is cleaner but am reluctant to have this
>> change.
> 
> Wait, do you actually depend on having these environment variables
> defined? Leaving environment variables unset instead of initializing
> them to an empty string shouldn't make any difference if you use a shell
> script (unless you check for it explicitly or do some fancy stuff).
> 

unless you use 'set -u', which I do.

> If you use C (other any other programming language that uses getenv() or
> behaves similarly), making your code compatible is as easy as modifying
> the NULL check branch after getenv() to set the result to an empty
> string instead of bailing out. It literally is a one-line diff.
> 
> Given that this is a real improvement and we only break backwards
> compatibility for a few corner cases, +1 to changing this here.

>From my point of view it is an ABI change...

Granted, there don't seem to be many users of this yet but an ABI change
nonetheless



-- 
Ferry Huberts