Re: [PATCH] Fix possible underflow in expression (maxoff - 1)

On Sun, Nov 24, 2019 at 11:21 AM Ranier Vilela <ranier_gyn(at)hotmail(dot)com> wrote:
> >In general, it's not possible to split a page without it being
> >initialized, and having at least 2 items (not including the incoming
> >newitem). Besides, even if "maxoff" had an integer underflow the
> >behavior of the function would still be sane and defined. OffsetNumber
> >is an unsigned type.
> Well, I didn't mean that it's failing..I meant it could fail..
> If PageGetMaxOffsetNumber, can return zero, maxoff can be zero.
> (0 - 1), on unsigned type, certainly is underflow and if maxoff can be one,
> (1 - 1) is zero, and state->newitemsz * (maxoff - 1), is zero.

I think that you're being far too optimistic about your ability to
detect and report valid issues using these static analysis tools. It's
not possible to apply the information they provide without a high
level understanding of the design of the code. There are already quite
a few full time Postgres hackers that use tools like Coverity all the
time.

While it's certainly true that PageGetMaxOffsetNumber cannot in
general be trusted to be > 0, we're talking about code that exists to
deal with pages that are already full, and need to be split. It is
impossible for "maxoff" to underflow, even if you deliberately corrupt
a page image using a tool like pg_hexedit. Even if we failed to be
sufficiently defensive about such a case (which is not the case), it
wouldn't make any sense to fix it in this specific esoteric function,
which is called when we've already decided to split the page (but only
sometimes). Sanitization needs to happen at some central choke point.

> Yes,two static tools, but reviewed by me.

I strongly suggest confining all of this to a single thread, and
stating your reasoning upfront.

--
Peter Geoghegan