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
From 64ba49861b81dd1a71aa0ff1817c737eb441cbe3 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 17 Aug 2026 11:39:11 +0900 Subject: [PATCH v3] hstore: Rework module to use Size and {add,mul}_size for overflows --- contrib/hstore/hstore.h | 35 ++++++++++------ contrib/hstore/hstore_compat.c | 29 ++++++------- contrib/hstore/hstore_io.c | 51 +++++++++++++++-------- contrib/hstore/hstore_op.c | 12 +++--- contrib/hstore/hstore_subs.c | 4 +- contrib/hstore_plperl/hstore_plperl.c | 6 +-- contrib/hstore_plpython/hstore_plpython.c | 6 +-- 7 files changed, 84 insertions(+), 59 deletions(-) diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h index 897af244a423..39cbe4d1010c 100644 --- a/contrib/hstore/hstore.h +++ b/contrib/hstore/hstore.h @@ -62,14 +62,23 @@ typedef struct #define HS_SETCOUNT(hsp_,c_) ((hsp_)->size_ = (c_) | HS_FLAG_NEWVERSION) -/* - * "x" comes from an existing HS_COUNT() (as discussed, <= INT_MAX/24) or a - * Pairs array length (due to MaxAllocSize, <= INT_MAX/40). "lenstr" is no - * more than INT_MAX, that extreme case arising in hstore_from_arrays(). - * Therefore, this calculation is limited to about INT_MAX / 5 + INT_MAX. - */ #define HSHRDSIZE (sizeof(HStore)) -#define CALCDATASIZE(x, lenstr) ( (x) * 2 * sizeof(HEntry) + HSHRDSIZE + (lenstr) ) + +/* + * "x" is a pair count, coming from an existing HS_COUNT() (as discussed, <= + * INT_MAX/24) or a Pairs array length (due to MaxAllocSize, <= INT_MAX/40). + * "lenstr" is no more than INT_MAX, that extreme case arising in + * hstore_from_arrays(). Therefore, this calculation should be limited to + * about INT_MAX / 5 + INT_MAX. + */ +static inline Size +hstoreCalcDataSize(Size x, Size lenstr) +{ + Size entrysize = mul_size(x, 2 * sizeof(HEntry)); + Size total = add_size(HSHRDSIZE, lenstr); + + return add_size(entrysize, total); +} /* note multiple evaluations of x */ #define ARRPTR(x) ( (HEntry*) ( (HStore*)(x) + 1 ) ) @@ -128,7 +137,7 @@ typedef struct /* finalize a newly-constructed hstore */ #define HS_FINALIZE(hsp_,count_,buf_,ptr_) \ do { \ - int _buflen = (ptr_) - (buf_); \ + Size _buflen = (ptr_) - (buf_); \ if ((count_)) \ ARRPTR(hsp_)[0].entry |= HENTRY_ISFIRST; \ if ((count_) != HS_COUNT((hsp_))) \ @@ -136,14 +145,14 @@ typedef struct HS_SETCOUNT((hsp_),(count_)); \ memmove(STRPTR(hsp_), (buf_), _buflen); \ } \ - SET_VARSIZE((hsp_), CALCDATASIZE((count_), _buflen)); \ + SET_VARSIZE((hsp_), hstoreCalcDataSize((count_), _buflen)); \ } while (0) /* ensure the varlena size of an existing hstore is correct */ #define HS_FIXSIZE(hsp_,count_) \ do { \ - int bl = (count_) ? HSE_ENDPOS(ARRPTR(hsp_)[2*(count_)-1]) : 0; \ - SET_VARSIZE((hsp_), CALCDATASIZE((count_),bl)); \ + Size bl = (count_) ? HSE_ENDPOS(ARRPTR(hsp_)[2*(count_)-1]) : 0; \ + SET_VARSIZE((hsp_), hstoreCalcDataSize((count_),bl)); \ } while (0) /* DatumGetHStoreP includes support for reading old-format hstore values */ @@ -168,8 +177,8 @@ typedef struct bool needfree; /* need to pfree the value? */ } Pairs; -extern PGDLLEXPORT int hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen); -extern PGDLLEXPORT HStore *hstorePairs(Pairs *pairs, int32 pcount, int32 buflen); +extern PGDLLEXPORT int hstoreUniquePairs(Pairs *a, int32 l, Size *buflen); +extern PGDLLEXPORT HStore *hstorePairs(Pairs *pairs, int32 pcount, Size buflen); extern PGDLLEXPORT size_t hstoreCheckKeyLen(size_t len); extern PGDLLEXPORT size_t hstoreCheckValLen(size_t len); diff --git a/contrib/hstore/hstore_compat.c b/contrib/hstore/hstore_compat.c index 3a9f7f45cb71..1e2db807edf7 100644 --- a/contrib/hstore/hstore_compat.c +++ b/contrib/hstore/hstore_compat.c @@ -90,8 +90,8 @@ * This is the structure used for entries in the old contrib/hstore * implementation. Notice that this is the same size as the new entry * (two 32-bit words per key/value pair) and that the header is the - * same, so the old and new versions of ARRPTR, STRPTR, CALCDATASIZE - * etc. are compatible. + * same, so the old and new versions of ARRPTR, STRPTR, + * hstoreCalcDataSize etc. are compatible. * * If the above statement isn't true on some bizarre platform, we're * a bit hosed. @@ -123,8 +123,8 @@ hstoreValidNewFormat(HStore *hs) { int count = HS_COUNT(hs); HEntry *entries = ARRPTR(hs); - int buflen = (count) ? HSE_ENDPOS(entries[2 * (count) - 1]) : 0; - int vsize = CALCDATASIZE(count, buflen); + Size buflen = (count) ? HSE_ENDPOS(entries[2 * (count) - 1]) : 0; + Size vsize = hstoreCalcDataSize(count, buflen); int i; if (hs->size_ & HS_FLAG_NEWVERSION) @@ -173,11 +173,11 @@ hstoreValidNewFormat(HStore *hs) static int hstoreValidOldFormat(HStore *hs) { - int count = hs->size_; + uint32 count = hs->size_; HOldEntry *entries = (HOldEntry *) ARRPTR(hs); - int vsize; - int lastpos = 0; - int i; + Size vsize; + Size lastpos = 0; + uint32 i; if (hs->size_ & HS_FLAG_NEWVERSION) return 0; @@ -188,7 +188,7 @@ hstoreValidOldFormat(HStore *hs) if (count > 0xFFFFFFF) return 0; - if (CALCDATASIZE(count, 0) > VARSIZE(hs)) + if (hstoreCalcDataSize(count, 0) > VARSIZE(hs)) return 0; if (entries[0].pos != 0) @@ -212,11 +212,12 @@ hstoreValidOldFormat(HStore *hs) { if (entries[i].pos != lastpos) return 0; - lastpos += (entries[i].keylen - + ((entries[i].valisnull) ? 0 : entries[i].vallen)); + lastpos = add_size(lastpos, + entries[i].keylen + + ((entries[i].valisnull) ? 0 : entries[i].vallen)); } - vsize = CALCDATASIZE(count, lastpos); + vsize = hstoreCalcDataSize(count, lastpos); if (vsize > VARSIZE(hs)) return 0; @@ -316,10 +317,10 @@ hstoreUpgrade(Datum orig) * must have an old-style value. Overwrite it in place as a new-style one. */ { - int count = hs->size_; + uint32 count = hs->size_; HEntry *new_entries = ARRPTR(hs); HOldEntry *old_entries = (HOldEntry *) ARRPTR(hs); - int i; + uint32 i; for (i = 0; i < count; ++i) { diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c index 9b72efb8674a..6fc29a211b25 100644 --- a/contrib/hstore/hstore_io.c +++ b/contrib/hstore/hstore_io.c @@ -349,6 +349,21 @@ comparePairs(const void *a, const void *b) return (pa->keylen > pb->keylen) ? 1 : -1; } +/* + * Add the string-data length of a pair to *buflen. + * + * This is a convenience routine for add_size(), checking if a Pair is null + * before adding its size. Individual keys and values are each limited to + * HENTRY_POSMASK bytes. + */ +static void +hstoreAddPairLen(Size *buflen, const Pairs *pair) +{ + Size pairlen = pair->keylen + (pair->isnull ? 0 : pair->vallen); + + *buflen = add_size(*buflen, pairlen); +} + /* * this code still respects pairs.needfree, even though in general * it should never be called in a context where anything needs freeing. @@ -356,7 +371,7 @@ comparePairs(const void *a, const void *b) * and (b) who knows whether they might be needed by some caller. */ int -hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen) +hstoreUniquePairs(Pairs *a, int32 l, Size *buflen) { Pairs *ptr, *res; @@ -365,7 +380,7 @@ hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen) if (l < 2) { if (l == 1) - *buflen = a->keylen + ((a->isnull) ? 0 : a->vallen); + hstoreAddPairLen(buflen, a); return l; } @@ -391,7 +406,7 @@ hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen) } else { - *buflen += res->keylen + ((res->isnull) ? 0 : res->vallen); + hstoreAddPairLen(buflen, res); res++; if (res != ptr) memcpy(res, ptr, sizeof(Pairs)); @@ -400,7 +415,7 @@ hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen) ptr++; } - *buflen += res->keylen + ((res->isnull) ? 0 : res->vallen); + hstoreAddPairLen(buflen, res); return res + 1 - a; } @@ -446,16 +461,17 @@ hstoreCheckValLength(size_t len, HSParser *state) HStore * -hstorePairs(Pairs *pairs, int32 pcount, int32 buflen) +hstorePairs(Pairs *pairs, int32 pcount, Size buflen) { HStore *out; HEntry *entry; char *ptr; char *buf; - int32 len; + Size len; int32 i; - len = CALCDATASIZE(pcount, buflen); + len = hstoreCalcDataSize(pcount, buflen); + out = palloc(len); SET_VARSIZE(out, len); HS_SETCOUNT(out, pcount); @@ -482,7 +498,7 @@ hstore_in(PG_FUNCTION_ARGS) char *str = PG_GETARG_CSTRING(0); Node *escontext = fcinfo->context; HSParser state; - int32 buflen; + Size buflen; HStore *out; state.begin = str; @@ -503,7 +519,7 @@ PG_FUNCTION_INFO_V1(hstore_recv); Datum hstore_recv(PG_FUNCTION_ARGS) { - int32 buflen; + Size buflen; HStore *out; Pairs *pairs; int32 i; @@ -602,7 +618,7 @@ PG_FUNCTION_INFO_V1(hstore_from_arrays); Datum hstore_from_arrays(PG_FUNCTION_ARGS) { - int32 buflen; + Size buflen; HStore *out; Pairs *pairs; Datum *key_datums; @@ -721,7 +737,7 @@ hstore_from_array(PG_FUNCTION_ARGS) ArrayType *in_array = PG_GETARG_ARRAYTYPE_P(0); int ndims = ARR_NDIM(in_array); int count; - int32 buflen; + Size buflen; HStore *out; Pairs *pairs; Datum *in_datums; @@ -835,7 +851,7 @@ Datum hstore_from_record(PG_FUNCTION_ARGS) { HeapTupleHeader rec; - int32 buflen; + Size buflen; HStore *out; Pairs *pairs; Oid tupType; @@ -1226,8 +1242,8 @@ Datum hstore_out(PG_FUNCTION_ARGS) { HStore *in = PG_GETARG_HSTORE_P(0); - int buflen, - i; + Size buflen; + int i; int count = HS_COUNT(in); char *out, *ptr; @@ -1250,11 +1266,10 @@ hstore_out(PG_FUNCTION_ARGS) for (i = 0; i < count; i++) { /* include "" and => and comma-space */ - buflen += 6 + 2 * HSTORE_KEYLEN(entries, i); + buflen = add_size(buflen, add_size(6, mul_size(2, HSTORE_KEYLEN(entries, i)))); /* include "" only if nonnull */ - buflen += 2 + (HSTORE_VALISNULL(entries, i) - ? 2 - : 2 * HSTORE_VALLEN(entries, i)); + buflen = add_size(buflen, HSTORE_VALISNULL(entries, i) ? 4 : + add_size(2, mul_size(2, HSTORE_VALLEN(entries, i)))); } out = ptr = palloc(buflen); diff --git a/contrib/hstore/hstore_op.c b/contrib/hstore/hstore_op.c index bcba75f92580..abc65ea62ff4 100644 --- a/contrib/hstore/hstore_op.c +++ b/contrib/hstore/hstore_op.c @@ -76,7 +76,7 @@ hstoreArrayToPairs(ArrayType *a, int *npairs) bool *key_nulls; int key_count; Pairs *key_pairs; - int bufsiz; + Size bufsiz; int i, j; @@ -637,7 +637,7 @@ hstore_slice_to_hstore(PG_FUNCTION_ARGS) int nkeys; Pairs *key_pairs = hstoreArrayToPairs(key_array, &nkeys); Pairs *out_pairs; - int bufsiz; + Size bufsiz; int lastidx = 0; int i; int out_count = 0; @@ -1241,8 +1241,8 @@ hstore_hash(PG_FUNCTION_ARGS) */ Assert(VARSIZE(hs) == (HS_COUNT(hs) != 0 ? - CALCDATASIZE(HS_COUNT(hs), - HSE_ENDPOS(ARRPTR(hs)[2 * HS_COUNT(hs) - 1])) : + hstoreCalcDataSize(HS_COUNT(hs), + HSE_ENDPOS(ARRPTR(hs)[2 * HS_COUNT(hs) - 1])) : HSHRDSIZE)); PG_FREE_IF_COPY(hs, 0); @@ -1264,8 +1264,8 @@ hstore_hash_extended(PG_FUNCTION_ARGS) /* See comment in hstore_hash */ Assert(VARSIZE(hs) == (HS_COUNT(hs) != 0 ? - CALCDATASIZE(HS_COUNT(hs), - HSE_ENDPOS(ARRPTR(hs)[2 * HS_COUNT(hs) - 1])) : + hstoreCalcDataSize(HS_COUNT(hs), + HSE_ENDPOS(ARRPTR(hs)[2 * HS_COUNT(hs) - 1])) : HSHRDSIZE)); PG_FREE_IF_COPY(hs, 0); diff --git a/contrib/hstore/hstore_subs.c b/contrib/hstore/hstore_subs.c index 56e0858c1a67..c1695daf72fe 100644 --- a/contrib/hstore/hstore_subs.c +++ b/contrib/hstore/hstore_subs.c @@ -192,7 +192,7 @@ hstore_subscript_assign(ExprState *state, HStore *hs = DatumGetHStoreP(*op->resvalue); int s1count = HS_COUNT(hs); int outcount = 0; - int vsize; + Size vsize; char *ps1, *bufd, *pd; @@ -202,7 +202,7 @@ hstore_subscript_assign(ExprState *state, int s2idx; /* Allocate result without considering possibility of duplicate */ - vsize = CALCDATASIZE(s1count + 1, VARSIZE(hs) + p.keylen + p.vallen); + vsize = hstoreCalcDataSize(s1count + 1, VARSIZE(hs) + p.keylen + p.vallen); out = palloc(vsize); SET_VARSIZE(out, vsize); HS_SETCOUNT(out, s1count + 1); diff --git a/contrib/hstore_plperl/hstore_plperl.c b/contrib/hstore_plperl/hstore_plperl.c index d7f1b8ddb488..4a6ff80dae54 100644 --- a/contrib/hstore_plperl/hstore_plperl.c +++ b/contrib/hstore_plperl/hstore_plperl.c @@ -13,9 +13,9 @@ PG_MODULE_MAGIC_EXT( /* Linkage to functions in hstore module */ typedef HStore *(*hstoreUpgrade_t) (Datum orig); static hstoreUpgrade_t hstoreUpgrade_p; -typedef int (*hstoreUniquePairs_t) (Pairs *a, int32 l, int32 *buflen); +typedef int (*hstoreUniquePairs_t) (Pairs *a, int32 l, Size *buflen); static hstoreUniquePairs_t hstoreUniquePairs_p; -typedef HStore *(*hstorePairs_t) (Pairs *pairs, int32 pcount, int32 buflen); +typedef HStore *(*hstorePairs_t) (Pairs *pairs, int32 pcount, Size buflen); static hstorePairs_t hstorePairs_p; typedef size_t (*hstoreCheckKeyLen_t) (size_t len); static hstoreCheckKeyLen_t hstoreCheckKeyLen_p; @@ -104,7 +104,7 @@ plperl_to_hstore(PG_FUNCTION_ARGS) SV *in = (SV *) PG_GETARG_POINTER(0); HV *hv; HE *he; - int32 buflen; + Size buflen; int32 i; int32 pcount; HStore *out; diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c index b9d8b4537f78..6e726805dfd8 100644 --- a/contrib/hstore_plpython/hstore_plpython.c +++ b/contrib/hstore_plpython/hstore_plpython.c @@ -19,9 +19,9 @@ static PLyUnicode_FromStringAndSize_t PLyUnicode_FromStringAndSize_p; /* Linkage to functions in hstore module */ typedef HStore *(*hstoreUpgrade_t) (Datum orig); static hstoreUpgrade_t hstoreUpgrade_p; -typedef int (*hstoreUniquePairs_t) (Pairs *a, int32 l, int32 *buflen); +typedef int (*hstoreUniquePairs_t) (Pairs *a, int32 l, Size *buflen); static hstoreUniquePairs_t hstoreUniquePairs_p; -typedef HStore *(*hstorePairs_t) (Pairs *pairs, int32 pcount, int32 buflen); +typedef HStore *(*hstorePairs_t) (Pairs *pairs, int32 pcount, Size buflen); static hstorePairs_t hstorePairs_p; typedef size_t (*hstoreCheckKeyLen_t) (size_t len); static hstoreCheckKeyLen_t hstoreCheckKeyLen_p; @@ -156,7 +156,7 @@ plpython_to_hstore(PG_FUNCTION_ARGS) PG_TRY(); { - int32 buflen; + Size buflen; Py_ssize_t i; Pairs *pairs; -- 2.55.0
signature.asc
Description: PGP signature
