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.
patch attached.
best regards,
Ranier Vilela
Attachments:
0001-Avoid-possible-overflow-with-ltsGetFreeBlock-functio.patchapplication/octet-stream; name=0001-Avoid-possible-overflow-with-ltsGetFreeBlock-functio.patchDownload
From f7c8ef7f55b64b8d594d1675a7dc183222729bc1 Mon Sep 17 00:00:00 2001
From: Ranier Vilela <ranier.vf@gmail.com>
Date: Thu, 24 Aug 2023 14:02:42 -0300
Subject: [PATCH] Avoid possible overflow with ltsGetFreeBlock function
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.
Author: Ranier vilela (ranier.vf@gmail.com)
---
src/backend/utils/sort/logtape.c | 2 +-
1files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 52b8898d5e..f31878bdee 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -372,7 +372,7 @@ ltsGetFreeBlock(LogicalTapeSet *lts)
{
long *heap = lts->freeBlocks;
long blocknum;
- int heapsize;
+ long heapsize;
long holeval;
unsigned long holepos;
--
2.32.0.windows.1
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:
/messages/by-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:
/messages/by-id/CAH2-Wzn5PCBLUrrds=hD439LtWP+PD7ekRTd=8LdtqJ+KO5D1Q@mail.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