From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, 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 20:04:44 -0500 [thread overview]
Message-ID: <X77/LEwWlz9fjbYM@nand.local> (raw)
In-Reply-To: <X777dl3pqioME7uM@coredump.intra.peff.net>
On Wed, Nov 25, 2020 at 07:48:54PM -0500, Jeff King wrote:
> 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.
Yeah, a find_revindex_entry() that returns NULL means that the caller is
probably dead in the water.
FWIW, this function gets touched by a series that I'm working on here:
[1]. There, I think "returning NULL" is equivalent to "returning -1",
and the problem exists there, too.
We could return a different negative number, call BUG(), or do nothing
other than what's written. I don't have any strong feelings, though.
> -Peff
Thanks,
Taylor
[1]: https://github.com/ttaylorr/git/blob/tb/on-disk-revindex-part-one/pack-revindex.c#L177-L201
next prev parent reply other threads:[~2020-11-26 1:04 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
2020-11-26 1:04 ` Taylor Blau [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X77/LEwWlz9fjbYM@nand.local \
--to=me@ttaylorr.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.