Обсуждение: Avoid a possible overflow (src/backend/utils/sort/logtape.c)
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.
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
Вложения
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
Вложения
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
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
Вложения
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
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
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
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
Вложения
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