From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 554DC1F86C for ; Thu, 26 Nov 2020 00:50:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728708AbgKZAsz (ORCPT ); Wed, 25 Nov 2020 19:48:55 -0500 Received: from cloud.peff.net ([104.130.231.41]:43200 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728593AbgKZAsz (ORCPT ); Wed, 25 Nov 2020 19:48:55 -0500 Received: (qmail 12360 invoked by uid 109); 26 Nov 2020 00:48:55 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 26 Nov 2020 00:48:55 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12317 invoked by uid 111); 26 Nov 2020 00:48:54 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 25 Nov 2020 19:48:54 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 25 Nov 2020 19:48:54 -0500 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , git@vger.kernel.org, dstolee@microsoft.com Subject: Re: [PATCH 0/2] midx: prevent against racily disappearing packs Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Nov 25, 2020 at 01:13:31PM -0800, Junio C Hamano wrote: > Taylor Blau writes: > > > Here are a couple of patches that Peff and I wrote after noticing a > > problem where racily disappearing .idx files can cause a segfault. While > > investigating, we fixed a related issue which is described and fixed in > > the second patch. > > In the cover letter it won't affect the end result, but when talking > about "race", it always is a good idea to explicitly mention both > sides of the race. It is clear what one side is in the above > (i.e. somebody who removes .idx file that is still in use), but it > is not so clear who gets hit and segfaults. > > I am guessing that the other party is the user of .pack file(s) > bypassing the corresponding .idx file(s) because the necessary data > is mostly in .midx? Yeah, the race reproduction in the second commit message can actually reproduce the segfault as well (it depends on the exact timing which error you get). So the segfault is in the reader, who is not checking the result of find_revindex_entry(). Arguably every call there should be checking for NULL, but in practice I think it would always be a bug: - we were somehow unable to open the index in order to generate the revindex (which is what happened here). But I think we are better off making sure that we can always do so, which is what this series does. - the caller asked about an object at a position beyond the number of objects in the packfile. This is a bug in the caller. So we could perhaps BUG() in find_revindex_entry() instead of returning NULL. A quick segfault accomplishes mostly the same thing, though the BUG() could distinguish the two cases more clearly. -Peff