On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin <a.okoshkin@xxxxxxxxxxx> 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?
Inspecting the four callsites:
* sequencer.c:
The prior lines to hold the index lock suggests we're not in a threaded
environment
* builtin/merge-recursive.c:
In cmd_merge_recursive and we're fiddling with argv/argc, which
suggests we in a main function, not having threads around
* builtin/am.c: fall_back_threeway called by am_run.
(am is not threaded)
* builtin/merge:
In try_merge_strategy called from the main function, also index locking
* builtin/checkout.c:
merge_working_tree also locks the index.
So I think this function is never called from within a threaded environment
in git.
>
> Signed-off-by: Andrey Okoshkin <a.okoshkin@xxxxxxxxxxx>
> ---
> merge-recursive.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 1494ffdb8..eaac98145 100644
> --- 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?
(The compiler may shuffle stuff around anyway, so this is a
moot suggestion; It gears mostly towards making the code more
readable/maintainable when presenting this part of the code
to the user.)
With or without this change:
Reviewed-by: Stefan Beller <sbeller@xxxxxxxxxx>
> 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
