[BUG] hstore integer overflow when constructing large values
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253418psql -h localhost -U postgresBuilt from patchset v5 (message #5), August 18, 2026 at 03:00 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253418_5 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253418_5 && git checkout t253418_5Patchset v5 (message #5) is on t253418_5
Hi,
My colleague Man Zeng reported an issue where hstore could fail with a
very large and unexpected memory allocation request. I investigated the
issue and found an integer overflow when constructing an hstore from
multiple large values.
For example:
CREATE EXTENSION hstore;
CREATE TABLE t(a text, b text, c text);
INSERT INTO t VALUES (repeat('x', 720000000), NULL, NULL);
UPDATE t SET b = repeat('y', 720000000);
UPDATE t SET c = repeat('z', 720000000);
SELECT hstore(t) FROM t;
This produces:
ERROR: invalid memory alloc request size 18446744071574584355
The problem is in `hstoreUniquePairs()`. `buflen` is an `int32`, and the
total length of all keys and values is accumulated into it without an
overflow check:
*buflen += res->keylen + ((res->isnull) ? 0 : res->vallen);
Each individual value in the example is within the hstore value length
limit, but their combined length is not. In this case, the accumulated
length is:
(720000000 + 1) * 3 = 2160000003
which exceeds `INT_MAX`. `buflen` therefore overflows before
`hstorePairs()` calculates the size of the resulting hstore. The negative
value eventually gets converted to `Size` when passed to `palloc()`,
resulting in the very large allocation size shown in the error message.
The attached patch checks the accumulated length before updating `buflen`,
so that an oversized hstore is rejected before the `int32` overflow can
occur. It also checks the complete size calculated by `CALCDATASIZE()`,
since the HEntry array and hstore header must fit in the resulting datum as
well.
I kept the existing `int32` arguments of `hstoreUniquePairs()` and
`hstorePairs()` unchanged rather than changing the exported interfaces.
The patch reports the size limit error consistently with the existing
hstore length checks.
I have not added the reproducer to the regression tests because it requires
constructing more than 2GB of input data. I couldn't find a reasonably
small SQL-level test case that exercises the same overflow.
Thanks to Man Zeng for reporting the issue.
Patch attached. Comments are welcome.
--
Thanks,
Tender Wang
On Sat, Aug 15, 2026 at 11:06 AM Tender Wang <tndrwang@gmail.com> wrote:
The attached patch checks the accumulated length before updating `buflen`,
so that an oversized hstore is rejected before the `int32` overflow can
occur. It also checks the complete size calculated by `CALCDATASIZE()`,
since the HEntry array and hstore header must fit in the resulting datum
as
well.
Hi,
I applied the patch and it applies cleanly on master, compiles without
warnings, and the hstore regression tests pass.
A minor comments:
The error message could be more informative, following the jsonb
precedent ("total size of jsonb array elements exceeds the maximum of
%u bytes"). Something like:
errmsg("total size of hstore data exceeds the maximum of %zu bytes",
(Size) MaxAllocSize)
I agree that a regression test isn't practical here, since triggering
the overflow requires more than 2GB of input; that's consistent with
the existing limits (e.g. HSTORE_MAX_KEY_LEN) not being exercised in
the tests either.
Tender Wang <tndrwang@gmail.com> 于2026年8月15日周六 11:20写道：
Show quoted text
Hi,
My colleague Man Zeng reported an issue where hstore could fail with a
very large and unexpected memory allocation request. I investigated the
issue and found an integer overflow when constructing an hstore from
multiple large values.For example:
CREATE EXTENSION hstore;
CREATE TABLE t(a text, b text, c text);INSERT INTO t VALUES (repeat('x', 720000000), NULL, NULL);
UPDATE t SET b = repeat('y', 720000000);
UPDATE t SET c = repeat('z', 720000000);SELECT hstore(t) FROM t;
This produces:
ERROR: invalid memory alloc request size 18446744071574584355The problem is in `hstoreUniquePairs()`. `buflen` is an `int32`, and the
total length of all keys and values is accumulated into it without an
overflow check:*buflen += res->keylen + ((res->isnull) ? 0 : res->vallen);
Each individual value in the example is within the hstore value length
limit, but their combined length is not. In this case, the accumulated
length is:
(720000000 + 1) * 3 = 2160000003which exceeds `INT_MAX`. `buflen` therefore overflows before
`hstorePairs()` calculates the size of the resulting hstore. The negative
value eventually gets converted to `Size` when passed to `palloc()`,
resulting in the very large allocation size shown in the error message.The attached patch checks the accumulated length before updating `buflen`,
so that an oversized hstore is rejected before the `int32` overflow can
occur. It also checks the complete size calculated by `CALCDATASIZE()`,
since the HEntry array and hstore header must fit in the resulting datum as
well.I kept the existing `int32` arguments of `hstoreUniquePairs()` and
`hstorePairs()` unchanged rather than changing the exported interfaces.The patch reports the size limit error consistently with the existing
hstore length checks.I have not added the reproducer to the regression tests because it requires
constructing more than 2GB of input data. I couldn't find a reasonably
small SQL-level test case that exercises the same overflow.Thanks to Man Zeng for reporting the issue.
Patch attached. Comments are welcome.
--
Thanks,
Tender Wang
On Sat, Aug 15, 2026 at 02:52:48PM +0800, Keyerror Smart wrote:
I agree that a regression test isn't practical here, since triggering
the overflow requires more than 2GB of input; that's consistent with
the existing limits (e.g. HSTORE_MAX_KEY_LEN) not being exercised in
the tests either.
+/*
+ * Add the string-data length of a pair to *buflen, checking for overflow.
+ * Individual keys and values are each limited to HENTRY_POSMASK bytes, but
+ * their combined length can exceed the range of int32.
+ */
+static void
+hstoreAddPairLen(int32 *buflen, const Pairs *pair)
+{
+ Size pairlen;
+
+ pairlen = pair->keylen + (pair->isnull ? 0 : pair->vallen);
+ if (pairlen > MaxAllocSize - (Size) *buflen)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("hstore is too large")));
+ *buflen += pairlen;
+}Switching to Size is what we should do, but the proposed patch is
doing it incorrectly and is actually incomplete. Let's rework all
this code so as we do not rely on int32 anymore for the calculated
length passed down to palloc(), and rely instead on add_size(), as
controlled by palloc.h and mcxt.c. What I mean here is to think more
deeply through this code rather than try to plug in weirdly one aspect
of the failures. (Bonus points: add_size() handles overflows and
complains about them.)
I doubt that I would backpatch any of that. There is nothing critical
as far as I understand, still it's a nice long-term improvement of the
allocation logic to use a Size where we can, going through the
palloc() overflow checks.
--
Michael
Michael Paquier <michael@paquier.xyz> 于2026年8月16日周日 14:45写道：
Switching to Size is what we should do, but the proposed patch is
doing it incorrectly and is actually incomplete. Let's rework all
this code so as we do not rely on int32 anymore for the calculated
length passed down to palloc(), and rely instead on add_size(), as
controlled by palloc.h and mcxt.c. What I mean here is to think more
deeply through this code rather than try to plug in weirdly one aspect
of the failures. (Bonus points: add_size() handles overflows and
complains about them.)I doubt that I would backpatch any of that. There is nothing critical
as far as I understand, still it's a nice long-term improvement of the
allocation logic to use a Size where we can, going through the
palloc() overflow checks.
Thanks for the review.
The reason I tried to keep `int32` for `buflen` in v1 was that
`hstoreUniquePairs()` and `hstorePairs()` are exported functions. I was
trying to keep the change minimally invasive in case the fix needed to be
backpatched, and therefore avoided changing their argument types.
Given that this does not seem worth backpatching, I agree that there is no
good reason to preserve the existing `int32`-based size calculations.
I've reworked the patch to use `Size` throughout the allocation-size
calculation instead.
In v2:
* `buflen` and the final allocation length are changed to `Size`, including
the callers of `hstoreUniquePairs()` and `hstorePairs()`.
* The accumulation of key/value lengths uses `add_size()` instead of
unchecked integer arithmetic.
* `CALCDATASIZE()` now uses checked `Size` arithmetic with `add_size()` and
`mul_size()`, rather than relying on assumptions about the maximum values
of its arguments.
* Other size calculations on the same paths, including `HS_FINALIZE()`,
`HS_FIXSIZE()`, and the output buffer calculation in `hstore_out()`, have
also been adjusted to avoid using `int`/`int32` for allocation sizes.
With these changes, the original reproducer now reports:
ERROR: invalid memory alloc request size 2160000035
rather than:
ERROR: invalid memory alloc request size 18446744071574584355
The former is the actual size of the hstore representation being passed to
`palloc()`: 2160000003 bytes of key/value data plus 32 bytes for the hstore
header and HEntry array. Thus the size calculation no longer overflows an
`int32`, and the allocation limit is handled by the normal palloc machinery.
Attached is v2.
--
Thanks,
Tender Wang
On Mon, Aug 17, 2026 at 10:15:15AM +0800, Tender Wang wrote:
The reason I tried to keep `int32` for `buflen` in v1 was that
`hstoreUniquePairs()` and `hstorePairs()` are exported functions. I was
trying to keep the change minimally invasive in case the fix needed to be
backpatched, and therefore avoided changing their argument types.Given that this does not seem worth backpatching, I agree that there is no
good reason to preserve the existing `int32`-based size calculations.
I've reworked the patch to use `Size` throughout the allocation-size
calculation instead.
While looking at that, I am reaching similar conclusions in terms of
CALCDATASIZE(), and your version feels weird by having both a static
inline function *and* a macro.. Your previous hstoreAddPairLen() is
also tempting to keep. We apply the same rule in three places based
on if a pair is null or not.
The former is the actual size of the hstore representation being passed to
`palloc()`: 2160000003 bytes of key/value data plus 32 bytes for the hstore
header and HEntry array. Thus the size calculation no longer overflows an
`int32`, and the allocation limit is handled by the normal palloc machinery.
Attached is presumably what I would do, which has some similarities
with your v2, but it's a bit more expanded.
Please note some of the changes in hstore_compat.c, which happen due
to size_ being a uint32 but we decided to cast that to an int. I have
switched them while looking for patterns where a buflen was involved.
It does not seem reachable in practice, but as I'm looking at that
now, I may as well make that more consistent. Peter E. has done some
work that feels a bit familiar in e615da8cb21b, but these were under
the assumption that the end loop checks did not match with the
original ones. Here I'm changing both the counters and the end check
from one thing to the other, consistent with size_.
By the way, putting the attached patch aside for a second, I have
scratching my head for a bit to find out why your scenario was
failing. But that was due to -DWRITE_READ_PARSE_PLAN_TREES
-DCOPY_PARSE_PLAN_TREES..
--
Michael
Michael Paquier <michael@paquier.xyz> 于2026年8月17日周一 10:42写道：
While looking at that, I am reaching similar conclusions in terms of
CALCDATASIZE(), and your version feels weird by having both a static
inline function *and* a macro.. Your previous hstoreAddPairLen() is
also tempting to keep. We apply the same rule in three places based
on if a pair is null or not.
Agree
Attached is presumably what I would do, which has some similarities
with your v2, but it's a bit more expanded.
Thanks for the patch. I went through v3, and I think it is better and more
complete than my v2.
There is just one thing I noticed in `hstore_subscript_assign()`:
```
vsize = hstoreCalcDataSize(s1count + 1,
VARSIZE(hs) + p.keylen + p.vallen);
```
Here `VARSIZE(hs) + p.keylen + p.vallen` is still calculated using
unchecked additions before being passed to `hstoreCalcDataSize()`.
Given that the rest of the patch is moving allocation-size arithmetic to
`Size` and `add_size()`/`mul_size()`, I wonder if this should use
`add_size()` as well.
Perhaps the existing size limits make an overflow there unreachable in
practice, but using `add_size()` would seem more consistent with the rest
of this patch.
Other than that, v3 looks good to me.
--
Thanks,
Tender Wang
On Mon, Aug 17, 2026 at 11:06:17AM +0800, Tender Wang wrote:
Given that the rest of the patch is moving allocation-size arithmetic to
`Size` and `add_size()`/`mul_size()`, I wonder if this should use
`add_size()` as well.Perhaps the existing size limits make an overflow there unreachable in
practice, but using `add_size()` would seem more consistent with the rest
of this patch.
I was wondering about this part as well, but discarded it as not
reachable in practice. So it should not matter at the end.
The nested calls in hstore_out() feel a bit inelegant as well written
this way. Something in sequential form would feel much easier to
parse, with one line for each add_size involved.
--
Michael
On Mon, Aug 17, 2026 at 01:05:22PM +0900, Michael Paquier wrote:
The nested calls in hstore_out() feel a bit inelegant as well written
this way. Something in sequential form would feel much easier to
parse, with one line for each add_size involved.
Cleaned up a bit all that, and applied on HEAD.
--
Michael
Michael Paquier <michael@paquier.xyz> 于2026年8月18日周二 13:37写道：
On Mon, Aug 17, 2026 at 01:05:22PM +0900, Michael Paquier wrote:
The nested calls in hstore_out() feel a bit inelegant as well written
this way. Something in sequential form would feel much easier to
parse, with one line for each add_size involved.Cleaned up a bit all that, and applied on HEAD.
Thanks for pushing.
--
Thanks,
Tender Wang
On Tue, Aug 18, 2026 at 1:37 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Aug 17, 2026 at 01:05:22PM +0900, Michael Paquier wrote:
The nested calls in hstore_out() feel a bit inelegant as well written
this way. Something in sequential form would feel much easier to
parse, with one line for each add_size involved.Cleaned up a bit all that, and applied on HEAD.
Hi.
https://git.postgresql.org/cgit/postgresql.git/commit/?id=92e20d4a4382810a30bdb394473d6ee891edab73
The consensus seems favoring "size_t" over "Size" for new code, see [1]/messages/by-id/688312.1782222788@sss.pgh.pa.us, [2]/messages/by-id/CANWCAZZJxgDAUsBK-1CctczwTz2TVo3TzjhhSD7fAA9EWte3eA@mail.gmail.com,
though this doesn't appear to be documented anywhere.
[1]: /messages/by-id/688312.1782222788@sss.pgh.pa.us
[2]: /messages/by-id/CANWCAZZJxgDAUsBK-1CctczwTz2TVo3TzjhhSD7fAA9EWte3eA@mail.gmail.com
On Tue, Aug 18, 2026 at 01:54:38PM +0800, jian he wrote:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=92e20d4a4382810a30bdb394473d6ee891edab73
The consensus seems favoring "size_t" over "Size" for new code, see [1], [2],
though this doesn't appear to be documented anywhere.
Ugh. c.h says that size_t is just Size, so it's stylistic. I don't
feel strongly one way or another, and hstore tends to prefer camel
case historically.
--
Michael