From: Jeff King <peff@peff•net>
To: Derrick Stolee <derrickstolee@github•com>
Cc: Taylor Blau <me@ttaylorr•com>,
git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>,
Abhradeep Chakraborty <chakrabortyabhradeep79@gmail•com>
Subject: Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation
Date: Fri, 24 Mar 2023 14:29:29 -0400 [thread overview]
Message-ID: <20230324182929.GA536252@coredump.intra.peff.net> (raw)
In-Reply-To: <61ae4ad5-4d4d-933c-a2cb-e7e2cd734401@github.com>
On Fri, Mar 24, 2023 at 02:04:05PM -0400, Derrick Stolee wrote:
> >> + if (bitmap_git->map_pos >= bitmap_git->map_size)
> >> + BUG("bitmap position exceeds size (%"PRIuMAX" >= %"PRIuMAX")",
> >> + (uintmax_t)bitmap_git->map_pos,
> >> + (uintmax_t)bitmap_git->map_size);
> >
> > This uses ">=", which is good, because it is valid to walk the pointer
> > to one past the end of the map, which is effectively EOF. But as above,
> > in that condition the callers should be checking for this EOF state
> > before reading the bytes.
>
> In other words, it would be too easy for a strange data shape to trigger
> this BUG() statement, which should be reserved for the most-extreme cases
> of developer mistake. Things like "this is an unacceptable 'whence' value"
> or "this should never be NULL" make sense, but this is too likely to be
> hit due to a runtime error.
Sort of. AFAICT in all of the "increment" cases we'll already have done
bounds-checks, so this really is a BUG() condition. But in that case I
question whether it's even worthwhile. The calling code ends up being
something like:
/* check that we have enough bytes */
if (total - pos < 4)
return error(...);
/* read those bytes */
get_be32() or whatever...
/* now advance pos, making sure we...had enough bytes? */
bitmap_index_seek(..., 4);
We know the advance will succeed because we checked ahead of time that
we had enough bytes. So it really is a BUG() if we don't, as it would
indicate somebody missed the earlier check. On the other hand, it is a
weird spot for an extra check, because by definition we'll have just
read off the array just before the seek.
> Would it make sense to return an 'int' instead of the size_t of map_pos?
> That way we could return in error if this is exceeded, and then all
> callers can respond "oh wait, that move would exceed the file size, so
> I should fail in my own way..."?
You can die() to avoid returning an error. But given that this is bitmap
code, and we can generally complete the operation even if it is broken
(albeit slower), usually we'd try to return the error up the call chain
(like bitmap_index_seek_commit() does later in the series). Plus that
follows our libification trend of not killing the process in low-level
code.
It does make the callers more complicated, though. If this were
_replacing_ the existing bounds-checks that might be worth it, but
coming after like this, I guess I just don't see it as adding much.
The case where we _do_ seek directly to a file-provided offset, rather
than incrementing, is an important check that this series adds, but that
one should be a die() and not a BUG().
-Peff
next prev parent reply other threads:[~2023-03-24 18:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 20:02 [PATCH 0/6] pack-bitmap: miscellaneous mmap read hardening Taylor Blau
2023-03-20 20:02 ` [PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()` Taylor Blau
2023-03-21 17:35 ` Jeff King
2023-03-24 17:52 ` Derrick Stolee
2023-03-20 20:02 ` [PATCH 2/6] pack-bitmap.c: hide bitmap internals in `read_be32()` Taylor Blau
2023-03-20 20:02 ` [PATCH 3/6] pack-bitmap.c: drop unnecessary 'inline's Taylor Blau
2023-03-21 17:40 ` Jeff King
2023-03-20 20:02 ` [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation Taylor Blau
2023-03-21 17:56 ` Jeff King
2023-03-24 18:04 ` Derrick Stolee
2023-03-24 18:29 ` Jeff King [this message]
2023-03-24 23:23 ` Taylor Blau
2023-03-25 4:57 ` Jeff King
2023-03-24 23:13 ` Taylor Blau
2023-03-24 23:24 ` Taylor Blau
2023-03-24 23:08 ` Taylor Blau
2023-03-20 20:02 ` [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible Taylor Blau
2023-03-21 18:05 ` Jeff King
2023-03-24 18:06 ` Derrick Stolee
2023-03-24 18:35 ` Jeff King
2023-03-24 19:43 ` Junio C Hamano
2023-03-24 20:37 ` Jeff King
2023-03-24 21:38 ` Junio C Hamano
2023-03-24 22:57 ` Taylor Blau
2023-03-20 20:02 ` [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()` Taylor Blau
2023-03-21 18:13 ` Jeff King
2023-03-21 18:16 ` Taylor Blau
2023-03-21 18:27 ` Jeff King
2023-03-24 18:09 ` Derrick Stolee
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=20230324182929.GA536252@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=chakrabortyabhradeep79@gmail$(echo .)com \
--cc=derrickstolee@github$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox