Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once

On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote:

On 24 October 2017 at 18:45, Eric Sunshine [off-list ref] wrote:

quoted

On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller [off-list ref] wrote:

quoted

On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin [off-list ref] wrote:

quoted

Add check of 'GIT_MERGE_VERBOSITY' environment variable only once in
init_merge_options().
Consequential call of getenv() may return NULL pointer and strtol() crashes.
However the stored pointer to the obtained getenv() result may be invalidated
by some other getenv() call from another thread as getenv() is not thread-safe.

I'm having trouble wrapping my head around this. Under which
circumstances could the second call in the current code return NULL, but
the code after your patch behave in a well-defined (and correct) way?

Yeah, it's not at all clear to me this is solving a real problem. I know
Andrey mentioned playing around with fault injection in an earlier
thread, so I'm wondering if there is an artificial fault being injected
into the second getenv() call. Which does not seem like something that
should be possible in the real world.

I definitely agree with the sentiment that as few things as possible
should happen between calling getenv() and using its result. I've seen
real bugs there from unexpected invalidation of the static buffer.

-Peff