From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.6 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 79A381FF72 for ; Tue, 24 Oct 2017 19:52:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751409AbdJXTwY (ORCPT ); Tue, 24 Oct 2017 15:52:24 -0400 Received: from cloud.peff.net ([104.130.231.41]:34670 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751330AbdJXTwY (ORCPT ); Tue, 24 Oct 2017 15:52:24 -0400 Received: (qmail 30154 invoked by uid 109); 24 Oct 2017 19:52:23 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 24 Oct 2017 19:52:23 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 14982 invoked by uid 111); 24 Oct 2017 19:52:30 -0000 Received: from Unknown (HELO sigill.intra.peff.net) (10.0.1.3) by peff.net (qpsmtpd/0.94) with SMTP; Tue, 24 Oct 2017 15:52:30 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 24 Oct 2017 12:52:21 -0700 Date: Tue, 24 Oct 2017 12:52:21 -0700 From: Jeff King To: Martin =?utf-8?B?w4VncmVu?= Cc: Eric Sunshine , Stefan Beller , Andrey Okoshkin , "git@vger.kernel.org" , vmiklos@frugalware.org, Junio C Hamano Subject: Re: [PATCH] merge-recursive: check GIT_MERGE_VERBOSITY only once Message-ID: <20171024195221.gqgtibwjaztgeel6@sigill.intra.peff.net> References: <3aed764b-388c-d163-08fc-32b294c6b9d3@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Oct 24, 2017 at 07:11:24PM +0200, Martin Ågren wrote: > On 24 October 2017 at 18:45, Eric Sunshine wrote: > > On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller wrote: > >> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin 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