Horton

Thread

Commits

  1. Avoid possible overflow with ltsGetFreeBlock() in logtape.c

  1. Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-08-24T17:46:42Z

    Hi,

nFreeBlocks stores the number of free blocks and
your type is *long*.

At Function ltsGetFreeBlock is locally stored in
heapsize wich type is *int*

With Windows both *long* and *int* are 4 bytes.
But with Linux *long* is 8 bytes and *int* are 4 bytes.

patch attached.

best regards,
Ranier Vilela

  2. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Michael Paquier <michael@paquier.xyz> — 2023-08-24T23:47:39Z

    On Thu, Aug 24, 2023 at 02:46:42PM -0300, Ranier Vilela wrote:
> With Windows both *long* and *int* are 4 bytes.
> But with Linux *long* is 8 bytes and *int* are 4 bytes.

And I recall that WIN32 is the only place where we treat long as 4
bytes.

> patch attached.

Yeah, it looks like you're right here.  Will do something about that.
--
Michael

  3. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Peter Geoghegan <pg@bowt.ie> — 2023-08-25T00:33:15Z

    On Thu, Aug 24, 2023 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:
> > patch attached.
>
> Yeah, it looks like you're right here.  Will do something about that.

This is a known issue. It has been discussed before.

I am in favor of fixing the problem. I don't quite recall what it was
that made the discussion stall last time around.

-- 
Peter Geoghegan

  4. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Michael Paquier <michael@paquier.xyz> — 2023-08-25T01:18:48Z

    On Thu, Aug 24, 2023 at 05:33:15PM -0700, Peter Geoghegan wrote:
> I am in favor of fixing the problem. I don't quite recall what it was
> that made the discussion stall last time around.

I think that you mean this one:
https://www.postgresql.org/message-id/CAH2-WznCscXnWmnj=STC0aSa7QG+BRedDnZsP=Jo_R9GUZvUrg@mail.gmail.com

Still that looks entirely different to me.  Here we have a problem
where the number of free blocks stored may cause an overflow in the
internal routine retrieving a free block, but your other thread
is about long being not enough on Windows.  I surely agree that
there's an argument for improving this interface and remove its use of
long in the long-term but that would not be backpatched.  I also don't
see why we cannot do the change proposed here until then, and it's
backpatchable.

There is a second thread related to logtape.c here, but that's still
different:
https://www.postgresql.org/message-id/flat/CAH2-Wzn5PCBLUrrds%3DhD439LtWP%2BPD7ekRTd%3D8LdtqJ%2BKO5D1Q%40mail.gmail.com
--
Michael

  5. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    David Rowley <dgrowleyml@gmail.com> — 2023-08-25T01:43:47Z

    On Fri, 25 Aug 2023 at 13:19, Michael Paquier <michael@paquier.xyz> wrote:
> Still that looks entirely different to me.  Here we have a problem
> where the number of free blocks stored may cause an overflow in the
> internal routine retrieving a free block, but your other thread
> is about long being not enough on Windows.  I surely agree that
> there's an argument for improving this interface and remove its use of
> long in the long-term but that would not be backpatched.  I also don't
> see why we cannot do the change proposed here until then, and it's
> backpatchable.

I agree with this. I think Ranier's patch is good and we should apply
it and backpatch it.

We shouldn't delay fixing this simple bug because we have some future
ambitions to swap the use of longs to int64.

David

  6. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Peter Geoghegan <pg@bowt.ie> — 2023-08-25T01:55:00Z

    On Thu, Aug 24, 2023 at 6:18 PM Michael Paquier <michael@paquier.xyz> wrote:
> Still that looks entirely different to me.  Here we have a problem
> where the number of free blocks stored may cause an overflow in the
> internal routine retrieving a free block, but your other thread
> is about long being not enough on Windows.

I must have seen logtape.c, windows, and long together on this thread,
and incorrectly surmised that it was exactly the same issue as before.
I now see that the only sense in which Windows is relevant is that
Windows happens to not have the same inconsistency. Windows is
consistently wrong.

So, yeah, I guess it's a different issue. Practically speaking it
should be treated as a separate issue, in any case. Since, as you
pointed out, there is no reason to not just fix this while
backpatching.

-- 
Peter Geoghegan

  7. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Peter Geoghegan <pg@bowt.ie> — 2023-08-25T02:02:40Z

    On Thu, Aug 24, 2023 at 6:44 PM David Rowley <dgrowleyml@gmail.com> wrote:
> I agree with this. I think Ranier's patch is good and we should apply
> it and backpatch it.

FWIW I'm pretty sure that it's impossible to run into problems here in
practice -- the minheap is allocated by palloc(), and the high
watermark number of free pages is pretty small. Even still, I agree
with your conclusion. There is really no reason to not be consistent
here.

-- 
Peter Geoghegan

  8. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Michael Paquier <michael@paquier.xyz> — 2023-08-29T23:05:57Z

    On Thu, Aug 24, 2023 at 07:02:40PM -0700, Peter Geoghegan wrote:
> FWIW I'm pretty sure that it's impossible to run into problems here in
> practice -- the minheap is allocated by palloc(), and the high
> watermark number of free pages is pretty small. Even still, I agree
> with your conclusion. There is really no reason to not be consistent
> here.

Postgres 16 RC1 is now tagged, so applied down to 13.
--
Michael

  9. Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-08-30T10:40:10Z

    Em ter., 29 de ago. de 2023 às 20:06, Michael Paquier <michael@paquier.xyz>
escreveu:

> On Thu, Aug 24, 2023 at 07:02:40PM -0700, Peter Geoghegan wrote:
> > FWIW I'm pretty sure that it's impossible to run into problems here in
> > practice -- the minheap is allocated by palloc(), and the high
> > watermark number of free pages is pretty small. Even still, I agree
> > with your conclusion. There is really no reason to not be consistent
> > here.
>
> Postgres 16 RC1 is now tagged, so applied down to 13.
>
Thank you, Michael.

best regards,
Ranier Vilela