Possible integer overflow in bringetbitmap()
Hello.
Function bringetbitmap() in src/backend/access/brin/brin.c:560 returns
type int64. But the returned variable 'totalpages' is of type int. Maybe
it makes sense to change the type of variable 'totalpages' to int64 to
avoid possible overflow in expression 'totalpages * 10'?
Patch is included in the attachment.
Best regards,
Evgeniy Gorbanyov
Attachments:
Possible-integer-overflow-in-bringetbitmap.patchtext/x-patch; charset=UTF-8; name=Possible-integer-overflow-in-bringetbitmap.patchDownload+1-1
On Tue, Nov 26, 2024 at 2:57 AM Evgeniy Gorbanyov
<gorbanyoves@basealt.ru> wrote:
Function bringetbitmap() in src/backend/access/brin/brin.c:560 returns type int64. But the returned variable 'totalpages' is of type int. Maybe it makes sense to change the type of variable 'totalpages' to int64 to avoid possible overflow in expression 'totalpages * 10'?
It looks to me like "totalpages" can never be larger than "nblocks",
which is of type BlockNumber, an alias for uint32.
The reason the function returns int64 is because it multiplies
"totalpages" by 10. However, notice that the existing line:
return totalpages * 10;
-- doesn't cast either "totalpages" or "10" to "int64", so the result
is of type uint32 again, which leads to integer overflow.
So, I think your patch should be, instead, to cast either "totalpages"
or "10" to "int64", in the return statement.
On Wed, Dec 04, 2024 at 02:05:10PM -0800, James Hunter wrote:
So, I think your patch should be, instead, to cast either "totalpages"
or "10" to "int64", in the return statement.
totalpages is signed, and BlockNumber is unsigned. Hence in theory
you could always fall into a trap once totalpages gets higher than
(2^31 - 1), no? This is not going to be a problem in practice even if
the number of pages per range assigned to brin can be 1, still..
--
Michael
On Wed, Dec 4, 2024 at 10:13 PM Michael Paquier <michael@paquier.xyz> wrote:
totalpages is signed, and BlockNumber is unsigned. Hence in theory
you could always fall into a trap once totalpages gets higher than
(2^31 - 1), no? This is not going to be a problem in practice even if
the number of pages per range assigned to brin can be 1, still..
Good point -- so the fix should be something like: (a) make totalpages
a BlockNumber or uint32; (b) cast either "totalpages" or "10" to
int64, before returning the result?
James
On Thu, Dec 05, 2024 at 08:46:45AM -0800, James Hunter wrote:
On Wed, Dec 4, 2024 at 10:13 PM Michael Paquier <michael@paquier.xyz> wrote:
totalpages is signed, and BlockNumber is unsigned. Hence in theory
you could always fall into a trap once totalpages gets higher than
(2^31 - 1), no? This is not going to be a problem in practice even if
the number of pages per range assigned to brin can be 1, still..Good point -- so the fix should be something like: (a) make totalpages
a BlockNumber or uint32; (b) cast either "totalpages" or "10" to
int64, before returning the result?
Sure, you could do (a) and (b) together. It also seems to me that it
is just simpler to make totalpages a int64 to map automatically with
the result expected by the caller of bringetbitmap(), and we know that
based on MaxBlockNumber we'll never run out of bits.
--
Michael
On Tue, Dec 10, 2024 at 12:33:08PM +0900, Michael Paquier wrote:
Sure, you could do (a) and (b) together. It also seems to me that it
is just simpler to make totalpages a int64 to map automatically with
the result expected by the caller of bringetbitmap(), and we know that
based on MaxBlockNumber we'll never run out of bits.
That should be simple enough. Are you planning to send a proposal of
patch?
--
Michael
On Fri, Dec 20, 2024 at 3:22 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Dec 10, 2024 at 12:33:08PM +0900, Michael Paquier wrote:
Sure, you could do (a) and (b) together. It also seems to me that it
is just simpler to make totalpages a int64 to map automatically with
the result expected by the caller of bringetbitmap(), and we know that
based on MaxBlockNumber we'll never run out of bits.That should be simple enough. Are you planning to send a proposal of
patch?
Attached the proposed one-line fix.
James
Attachments:
0001-Fix-integer-overflow-in-bringetbitmap.patchapplication/octet-stream; name=0001-Fix-integer-overflow-in-bringetbitmap.patchDownload+1-2
On Fri, Jan 10, 2025 at 11:22:37AM -0800, James Hunter wrote:
Attached the proposed one-line fix.
Yes, that should be fine as-is based on the original topic.
--
Michael
On Fri, Jan 10, 2025 at 11:22:37AM -0800, James Hunter wrote:
Attached the proposed one-line fix.
Thanks, applied and backpatched down to v13. While on it, I've
checked all the other amgetbitmap callbacks and they all rely on an
int64 for the result they return. So these parts are OK.
--
Michael