On 24 October 2017 at 18:45, Eric Sunshine <sunsh...@sunshineco.com> wrote: > On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller <sbel...@google.com> wrote: >> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin <a.okosh...@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.
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? > 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. Yeah. The call should be immediately before `merge_verbosity` is used. Then, if a compiler wants to move the call, it has to do the work and prove that it's ok.
