From 6462238f0d7b7c15eb3f54c2108573cee8fb24ba Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Mon, 27 Mar 2017 14:52:19 -0300 Subject: [PATCH] Fix uninitialized memory propagation mistakes MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Valgrind complains that some uninitialized bytes are being passed around by the extended statistics code since commit 7b504eb282ca2f, as reported by Andres Freund. Silence it. Tomas Vondra submitted a patch which he verified to fix the complaints in his machine; however I messed with it a bit before pushing, so any remaining problems are likely my (Álvaro's) fault. Author: Tomas Vondra Discussion: https://postgr.es/m/20170325211031.4xxoptigqxm2emn2@alap3.anarazel.de --- src/backend/statistics/mvdistinct.c | 89 +++++++++++++++++------------ src/include/statistics/statistics.h | 7 +++ 2 files changed, 61 insertions(+), 35 deletions(-) diff --git a/src/backend/statistics/mvdistinct.c b/src/backend/statistics/mvdistinct.c index 5df4e29eec..6082ff01a9 100644 --- a/src/backend/statistics/mvdistinct.c +++ b/src/backend/statistics/mvdistinct.c @@ -161,10 +161,10 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct) Assert(ndistinct->type == STATS_NDISTINCT_TYPE_BASIC); /* - * Base size is base struct size, plus one base struct for each items, - * including number of items for each. + * Base size is size of scalar fields in the struct, plus one base struct + * for each item, including number of items for each. */ - len = VARHDRSZ + offsetof(MVNDistinct, items) + + len = VARHDRSZ + SizeOfMVNDistinct + ndistinct->nitems * (offsetof(MVNDistinctItem, attrs) + sizeof(int)); /* and also include space for the actual attribute numbers */ @@ -182,9 +182,13 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct) tmp = VARDATA(output); - /* Store the base struct values */ - memcpy(tmp, ndistinct, offsetof(MVNDistinct, items)); - tmp += offsetof(MVNDistinct, items); + /* Store the base struct values (magic, type, nitems) */ + memcpy(tmp, &ndistinct->magic, sizeof(uint32)); + tmp += sizeof(uint32); + memcpy(tmp, &ndistinct->type, sizeof(uint32)); + tmp += sizeof(uint32); + memcpy(tmp, &ndistinct->nitems, sizeof(uint32)); + tmp += sizeof(uint32); /* * store number of attributes and attribute numbers for each ndistinct @@ -224,49 +228,64 @@ MVNDistinct * statext_ndistinct_deserialize(bytea *data) { int i; - Size expected_size; + Size minimum_size; + MVNDistinct ndist; MVNDistinct *ndistinct; char *tmp; if (data == NULL) return NULL; - if (VARSIZE_ANY_EXHDR(data) < offsetof(MVNDistinct, items)) + /* we expect at least the basic fields of MVNDistinct struct */ + if (VARSIZE_ANY_EXHDR(data) < SizeOfMVNDistinct) elog(ERROR, "invalid MVNDistinct size %ld (expected at least %ld)", - VARSIZE_ANY_EXHDR(data), offsetof(MVNDistinct, items)); - - /* read the MVNDistinct header */ - ndistinct = (MVNDistinct *) palloc(sizeof(MVNDistinct)); + VARSIZE_ANY_EXHDR(data), SizeOfMVNDistinct); /* initialize pointer to the data part (skip the varlena header) */ tmp = VARDATA_ANY(data); - /* get the header and perform basic sanity checks */ - memcpy(ndistinct, tmp, offsetof(MVNDistinct, items)); - tmp += offsetof(MVNDistinct, items); - - if (ndistinct->magic != STATS_NDISTINCT_MAGIC) - elog(ERROR, "invalid ndistinct magic %d (expected %d)", - ndistinct->magic, STATS_NDISTINCT_MAGIC); - - if (ndistinct->type != STATS_NDISTINCT_TYPE_BASIC) - elog(ERROR, "invalid ndistinct type %d (expected %d)", - ndistinct->type, STATS_NDISTINCT_TYPE_BASIC); - - Assert(ndistinct->nitems > 0); + /* read the header fields and perform basic sanity checks */ + memcpy(&ndist.magic, tmp, sizeof(uint32)); + tmp += sizeof(uint32); + memcpy(&ndist.type, tmp, sizeof(uint32)); + tmp += sizeof(uint32); + memcpy(&ndist.nitems, tmp, sizeof(uint32)); + tmp += sizeof(uint32); + + if (ndist.magic != STATS_NDISTINCT_MAGIC) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("invalid ndistinct magic %08x (expected %08x)", + ndist.magic, STATS_NDISTINCT_MAGIC))); + if (ndist.type != STATS_NDISTINCT_TYPE_BASIC) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("invalid ndistinct type %d (expected %d)", + ndist.type, STATS_NDISTINCT_TYPE_BASIC))); + if (ndist.nitems == 0) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("invalid zero-length item array in MVNDistinct"))); /* what minimum bytea size do we expect for those parameters */ - expected_size = offsetof(MVNDistinct, items) + - ndistinct->nitems * (offsetof(MVNDistinctItem, attrs) + - sizeof(AttrNumber) * 2); + minimum_size = (SizeOfMVNDistinct + + ndist.nitems * (SizeOfMVNDistinctItem + + sizeof(AttrNumber) * 2)); + if (VARSIZE_ANY_EXHDR(data) < minimum_size) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("invalid MVNDistinct size %ld (expected at least %ld)", + VARSIZE_ANY_EXHDR(data), minimum_size))); - if (VARSIZE_ANY_EXHDR(data) < expected_size) - elog(ERROR, "invalid dependencies size %ld (expected at least %ld)", - VARSIZE_ANY_EXHDR(data), expected_size); - - /* allocate space for the ndistinct items */ - ndistinct = repalloc(ndistinct, offsetof(MVNDistinct, items) + - (ndistinct->nitems * sizeof(MVNDistinctItem))); + /* + * Allocate space for the ndistinct items (no space for each item's attnos: + * those live in bitmapsets allocated separately) + */ + ndistinct = palloc0(MAXALIGN(SizeOfMVNDistinct) + + (ndist.nitems * sizeof(MVNDistinctItem))); + ndistinct->magic = ndist.magic; + ndistinct->type = ndist.type; + ndistinct->nitems = ndist.nitems; for (i = 0; i < ndistinct->nitems; i++) { diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h index a15e39e1a3..91645bff4b 100644 --- a/src/include/statistics/statistics.h +++ b/src/include/statistics/statistics.h @@ -27,6 +27,9 @@ typedef struct MVNDistinctItem double ndistinct; /* ndistinct value for this combination */ Bitmapset *attrs; /* attr numbers of items */ } MVNDistinctItem; +/* size of the struct, excluding attribute list */ +#define SizeOfMVNDistinctItem \ + (offsetof(MVNDistinctItem, ndistinct) + sizeof(double)) /* A MVNDistinct object, comprising all possible combinations of columns */ typedef struct MVNDistinct @@ -37,6 +40,10 @@ typedef struct MVNDistinct MVNDistinctItem items[FLEXIBLE_ARRAY_MEMBER]; } MVNDistinct; +/* size of the struct excluding the items array */ +#define SizeOfMVNDistinct (offsetof(MVNDistinct, nitems) + sizeof(uint32)) + + extern MVNDistinct *statext_ndistinct_load(Oid mvoid); extern void BuildRelationExtStatistics(Relation onerel, double totalrows, -- 2.40.0