hstore: Rework module to use Size and {add,mul}_size for allocation sizes

hstore has been relying on a set of int/int32 to count the total length
of all the keys and values constructed, relying on the maximum values of
the arguments rather than proper bound checks.  As proved in the
reported thread, these counters could overflow and wrap, leading to
incorrect allocations.

This commit replaces the int/int32 values with a set of Size values,
with add_size() and mul_size() in charge of checking for overflows when
constructing hstore values before doing any allocation.

The changes that matter in terms of the report are mostly in
hstore_io.c.  hstore_compat.c is adjusted for consistency, to keep the
allocation maths uniform across the module.

This is an old bug.  A backpatch should perhaps be done but I cannot get
excited about a change like that in stable branches.

Reported-by: Man Zeng <zengman@halodbtech.com>
Author: Tender Wang <tndrwang@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAHewXNmfD6-9MDxgpRfTzjVj0mhBw3JMoNOHeTiPOjZGhNzd=g@mail.gmail.com

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);

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)
 		{

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;
@@ -1249,12 +1265,18 @@ hstore_out(PG_FUNCTION_ARGS)
 
 	for (i = 0; i < count; i++)
 	{
-		/* include "" and => and comma-space */
-		buflen += 6 + 2 * HSTORE_KEYLEN(entries, i);
+		/* include "", => and comma-space */
+		buflen = add_size(buflen, 6);
+		buflen = add_size(buflen, mul_size(2, HSTORE_KEYLEN(entries, i)));
+
 		/* include "" only if nonnull */
-		buflen += 2 + (HSTORE_VALISNULL(entries, i)
-					   ? 2
-					   : 2 * HSTORE_VALLEN(entries, i));
+		if (HSTORE_VALISNULL(entries, i))
+			buflen = add_size(buflen, 4);
+		else
+		{
+			buflen = add_size(buflen, 2);
+			buflen = add_size(buflen, mul_size(2, HSTORE_VALLEN(entries, i)));
+		}
 	}
 
 	out = ptr = palloc(buflen);

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);

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);

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;

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;