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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 24, 2017, 16:45 UTC
Message-ID: <CAPig+cRTL2amjsgdp1=T3GMZLa=favugOfnQw9XjWzC+U=v5Sw@mail.gmail.com>
In-Reply-To: <CAGZ79kaSZG9WriqX0SLbTnEDRr-4YdFRuK17+9wnxyrftfnMmg@mail.gmail.com>

On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines

> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin <a.okoshkin@samsung.com> wrote:
>> 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.
>
> But do we have other threads running at the time?

I feel uncomfortable about this change, not due to lack of thread safety, but due to the distance between the getenv() invocation and its point of use. See below for more detail.

Show 15 quoted lines

>> Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
>> ---
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options *o)
>>  void init_merge_options(struct merge_options *o)
>>  {
>> +       const char *merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
>
> Despite not being in a threaded environment, I wonder if we want to
> minimize the time between  calling getenv and the use of the result,
> i.e. declare merge_verbosity here, but assign it later, just before the
> condition?
>
> With or without this change:
> Reviewed-by: Stefan Beller <sbeller@google.com>

The distance between getenv() and the point where the value is actually used is a big concern due to not knowing what is or might be going on in called functions between the two points. According to [1], the value returned by getenv() could be invalidated by another call to getenv() (or setenv() or unsetenv() or putenv()), and we don't have guarantee that we're safe from such invalidation considering that this function calls out to others. For instance, after getenv() but before the value is used, init_merge_options() calls merge_recursive_config() which calls git_config() which calls git_xmerge_config(), and so on.

For this reason, I have difficulty endorsing this change as-is.

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html

Show 17 quoted lines

>>         memset(o, 0, sizeof(struct merge_options));
>>         o->verbosity = 2;
>>         o->buffer_output = 1;
>> @@ -2171,9 +2172,8 @@ void init_merge_options(struct merge_options *o)
>>         o->renormalize = 0;
>>         o->detect_rename = 1;
>>         merge_recursive_config(o);
>> -       if (getenv("GIT_MERGE_VERBOSITY"))
>> -               o->verbosity =
>> -                       strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
>> +       if (merge_verbosity)
>> +               o->verbosity = strtol(merge_verbosity, NULL, 10);
>>         if (o->verbosity >= 5)
>>                 o->buffer_output = 0;
>>         strbuf_init(&o->obuf, 0);
>> --
>> 2.14.3