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

[Lukas Fleischer]

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

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.

> 
> I can live with that but let's ask Lars what he thinks about this

Ack.