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
