24.10.2017 22:52, Jeff King wrote: > On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote: > >> On 24 October 2017 at 18:45, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >>> On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote: >>>> 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. >> >> 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 Thanks for your comments. Jeff is right: there were some artificial fault injections imitating valid failures of different functions (syscalls, libc and so on). And yes - in the real life there is no problems with current code as there are no other threads running. However it's not a good practice to double call getenv() with the same argument: * Code readability. * Still no guaranty that the second call will be valid: some linked library may be compromised or LD_PRELOADed with the aim to create a race with getenv(). I believe there is no profit doing it here but it's just an explanation. In my opinion, here it's ok to save the pointer returned from the single getnev() call doing as few actions as possible between getenv() and strtol() calls. I'll change the patch. -- Best regards, Andrey Okoshkin
