Horton

Thread

Commits

  1. Fix bogus use of "long" in AllocSetCheck()

  1. Fix bogus use of "long" in aset.c

    David Rowley <dgrowleyml@gmail.com> — 2025-10-29T23:07:08Z

    While reviewing another patch, I was playing around with a test case
to trigger a large memory allocation. I was doing this on Windows when
I got a nasty looking WARNING in a MEMORY_CONTEXT_CHECKING build:

create table t (a int);
insert into t values(1);
alter table t alter column a set (n_distinct = -1); -- all values distinct
analyze t;
update pg_class set reltuples = 1e9 where oid = 't'::regclass; -- hack
to make the table big
set work_mem = '4GB';
explain (summary on) select a from t except select a from t;

and got:
WARNING:  problem in alloc set ExecutorState: bad single-chunk
0000023DE7C98098 in block 0000023DE7C98070
WARNING:  problem in alloc set ExecutorState: bad single-chunk
0000023DE7C98098 in block 0000023DE7C98070

It turns out that AllocSetCheck() thinks "long" is a good datatype to
store the difference between 2 pointers. That's not going to work well
on 64-bit Windows as long is 32-bit.

I did also consider [u]intptr_t, but thought Size was better as that's
what chsize is.

Trivial fix attached.

David

  2. Re: Fix bogus use of "long" in aset.c

    Michael Paquier <michael@paquier.xyz> — 2025-10-29T23:40:54Z

    On Thu, Oct 30, 2025 at 12:07:08PM +1300, David Rowley wrote:
> It turns out that AllocSetCheck() thinks "long" is a good datatype to
> store the difference between 2 pointers. That's not going to work well
> on 64-bit Windows as long is 32-bit.

Nice find.  Your suggestion of using Size makes sense here, also for
your consistency argument.
--
Michael

  3. Re: Fix bogus use of "long" in aset.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-29T23:55:12Z

    David Rowley <dgrowleyml@gmail.com> writes:
> It turns out that AllocSetCheck() thinks "long" is a good datatype to
> store the difference between 2 pointers. That's not going to work well
> on 64-bit Windows as long is 32-bit.

Ooops.  Surprised we've not noticed this before.

> I did also consider [u]intptr_t, but thought Size was better as that's
> what chsize is.

Seems like it's important that the value be signed, so maybe ssize_t?
Or ptrdiff_t?

			regards, tom lane

  4. Re: Fix bogus use of "long" in aset.c

    David Rowley <dgrowleyml@gmail.com> — 2025-10-30T00:19:58Z

    On Thu, 30 Oct 2025 at 12:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I did also consider [u]intptr_t, but thought Size was better as that's
> > what chsize is.
>
> Seems like it's important that the value be signed, so maybe ssize_t?
> Or ptrdiff_t?

I did think about this a bit. I did think that signed would make sense
for a few reasons, e.g if somehow the block's freeptr was a lower
address than the block. Since this is code that's checking for the
sanity of the context, maybe we shouldn't assume that's true (although
it should always be true).

The reason I left it an unsigned type was that the check is doing: if
(chsize + ALLOC_CHUNKHDRSZ != blk_used), so we'd still catch this with
an unsigned type, even if it wrapped due to going negative due to a
bogus freeptr. Changing to a signed type would leave us with a few
tests comparing signed to unsigned types. I wanted to try and avoid
that. I thought about making chsize signed, but that is also used for
storing the return value of GetChunkSizeFromFreeListIdx(), which is
unsigned. I just don't quite see a combination of changing the types
that doesn't leave us comparing signed with unsigned types.  In any
case, nothing is checking for pointers being less or greater than
other pointers, so it's not like we'll miss catching something due to
not using signed types.

David

  5. Re: Fix bogus use of "long" in aset.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-30T00:24:46Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 30 Oct 2025 at 12:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Seems like it's important that the value be signed, so maybe ssize_t?
>> Or ptrdiff_t?

> The reason I left it an unsigned type was that the check is doing: if
> (chsize + ALLOC_CHUNKHDRSZ != blk_used), so we'd still catch this with
> an unsigned type, even if it wrapped due to going negative due to a
> bogus freeptr. Changing to a signed type would leave us with a few
> tests comparing signed to unsigned types.

Yeah, that's fair.  I'm content with what you have.

			regards, tom lane

  6. Re: Fix bogus use of "long" in aset.c

    David Rowley <dgrowleyml@gmail.com> — 2025-10-30T01:51:52Z

    On Thu, 30 Oct 2025 at 13:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The reason I left it an unsigned type was that the check is doing: if
> > (chsize + ALLOC_CHUNKHDRSZ != blk_used), so we'd still catch this with
> > an unsigned type, even if it wrapped due to going negative due to a
> > bogus freeptr. Changing to a signed type would leave us with a few
> > tests comparing signed to unsigned types.
>
> Yeah, that's fair.  I'm content with what you have.

Thanks for the reviews. I've pushed and backpatched to 13.

David