Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation

[Jeff King <peff@peff.net>]

On Fri, Mar 24, 2023 at 07:23:06PM -0400, Taylor Blau wrote:

> On Fri, Mar 24, 2023 at 02:29:29PM -0400, Jeff King wrote:
> > 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.
> 
> Here you claim that we want bitmap_index_seek_to() to call BUG() if we
> end up with map_pos >= map_size. But...

I think the paragraph above doesn't have enough context. I meant
incrementing the pos here (which is why "we checked ahead of time that
we had enough bytes"), in which case it is a BUG() (double-checking the
earlier check).

In a seek_to(), there is no previous check. We have to make sure the
requested offset is within bounds.

> > 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().
> 
> ...here you say that it should be a die().

Right, so that one would be a die(). Or better still, an error().

> I think it does depend on the context. When seeking directly to a
> position before reading something, die()-ing is appropriate. The case
> where you seek to a relative position to reflect that you just read
> something, a BUG() is appropriate.

Right, exactly. We are agreeing, I think.

> So really, I think you want something like this:
> 
>     static void bitmap_index_seek_set(struct bitmap_index *bitmap_git, size_t pos)
>     {
>       if (pos >= bitmap_git->map_size)
>         die(_("bitmap position exceeds size (%"PRIuMAX" >= %"PRIuMAX")"),
>             (uintmax_t)bitmap_git->map_pos,
>             (uintmax_t)bitmap_git->map_size);
> 
>       bitmap_git->map_pos = pos;
>     }
> 
>     static void bitmap_index_seek_ahead(struct bitmap_index *bitmap_git,
>                                         size_t offset)
>     {
>       if (bitmap_git->map_pos + offset >= bitmap_git->map_size)
>         BUG("cannot seek %"PRIuMAX" byte(s) ahead of %"PRIuMAX" "
>             "(%"PRIuMAX" >= %"PRIuMAX")",
>             (uintmax_t)offset,
>             (uintmax_t)bitmap_git->map_pos,
>             (uintmax_t)(bitmap_git->map_pos + offset),
>             (uintmax_t)bitmap_git->map_size);
> 
>       bitmap_git->map_pos += offset;
>     }
> 
> Does that match what you were thinking?

Yes, though I am of the opinion that the assertion in seek_ahead() is
largely pointless, simply because if it ever triggered we would already
have triggered undefined behavior. I'm not opposed to adding it if you
feel strongly, I just wouldn't bother myself (and instead would focus on
making the "do we have enough bytes to read" checks more consistent and
harder-to-get-wrong).

Seeking to exactly map_size in the seek_set() case (i.e., the "=" in
">=") is a little funny, but not illegal. Either way, you'd want to
check "and do we have N bytes to read from this offset" immediately
afterwards (and your series does), so that would catch any non-zero
reads there.

-Peff