Horton

Thread

Commits

  1. Fix potential integer overflow in bringetbitmap()

  1. Possible integer overflow in bringetbitmap()

    Evgeniy Gorbanev <gorbanyoves@basealt.ru> — 2024-11-26T10:56:50Z

    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

  2. Re: Possible integer overflow in bringetbitmap()

    James Hunter <james.hunter.pg@gmail.com> — 2024-12-04T22:05:10Z

    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.

  3. Re: Possible integer overflow in bringetbitmap()

    Michael Paquier <michael@paquier.xyz> — 2024-12-05T06:13:14Z

    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

  4. Re: Possible integer overflow in bringetbitmap()

    James Hunter <james.hunter.pg@gmail.com> — 2024-12-05T16:46:45Z

    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

  5. Re: Possible integer overflow in bringetbitmap()

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T03:33:08Z

    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

  6. Re: Possible integer overflow in bringetbitmap()

    Michael Paquier <michael@paquier.xyz> — 2024-12-20T23:22:22Z

    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

  7. Re: Possible integer overflow in bringetbitmap()

    James Hunter <james.hunter.pg@gmail.com> — 2025-01-10T19:22:37Z

    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

  8. Re: Possible integer overflow in bringetbitmap()

    Michael Paquier <michael@paquier.xyz> — 2025-01-12T00:11:14Z

    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

  9. Re: Possible integer overflow in bringetbitmap()

    Michael Paquier <michael@paquier.xyz> — 2025-01-14T06:28:17Z

    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