From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org, dstolee@microsoft.com
Subject: Re: [PATCH 0/2] midx: prevent against racily disappearing packs
Date: Wed, 25 Nov 2020 19:48:54 -0500 [thread overview]
Message-ID: <X777dl3pqioME7uM@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqblfldts4.fsf@gitster.c.googlers.com>
On Wed, Nov 25, 2020 at 01:13:31PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> 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
next prev parent reply other threads:[~2020-11-26 0:50 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-25 17:17 [PATCH 0/2] midx: prevent against racily disappearing packs Taylor Blau
2020-11-25 17:17 ` [PATCH 1/2] packfile.c: protect against disappearing indexes Taylor Blau
2020-11-25 21:15 ` Junio C Hamano
2020-11-25 17:17 ` [PATCH 2/2] midx.c: protect against disappearing packs Taylor Blau
2020-11-25 21:22 ` Junio C Hamano
2020-11-25 21:13 ` [PATCH 0/2] midx: prevent against racily " Junio C Hamano
2020-11-26 0:48 ` Jeff King [this message]
2020-11-26 1:04 ` Taylor Blau
2020-11-26 1:27 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X777dl3pqioME7uM@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).