Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2013, 06:47 UTC
Message-ID: <7va9soqg6p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8CZe=qyzmG_1vdLYp07OvkDAU4wYc8MN3et7WBVmMhJOQ@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 11 quoted lines

> On Sat, Jan 5, 2013 at 11:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I personally do not think a wrapper with limited slots is a healthy
>> direction to go.  Most places we use getenv() do not let the return
>> value live across their scope, and those that do should explicitly
>> copy the value away.  It's between validating that there is _no_ *env()
>> calls in the codepath between a getenv() call and the use of its
>> return value, and validating that there is at most 4 such calls there.
>> The former is much easier to verify and maintain, I think.
>
> I did not look carefully and was scared of 143 getenv calls. But with
> about 4 calls, yes it's best to do without the wrapper.

Just to make sure you did not misunderstand, the 4 (four) in my message is not about "4 calls among 143 are unsafe".

It was referring to the number of rotating slots your patch defined, which means

	val = getenv("FOO");
        ... random other code ...
        use(val)

is safe only if random other code makes less than 4 getenv() calls.

I didn't verify all of the call sites. It needs to be done with or without your wrapper patch. Without your wrapper, the validation needs to make sure "random other code" above does not make any getenv() call. With your wrapper, the validation needs to make sure "random other code" above does not make more than 4 such calls. My point was that the effort needed for both are about the same, so your wrapper does not buy us much.