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
