My "make installcheck" runs while completing the just-posted Valgrind Memcheck patch revealed seven new and newly-detected (due to tighter checking) memory errors. Proposed patches attached.
* SP-GiST moveLeafs() and doPickSplit() read past the end of a palloc These functions construct arrays of OffsetNumber in palloc'd chunks, which they then place in WAL records. The rdata entry is done with a MAXALIGN'd size, but the associated palloc request may not extend to the next MAXALIGN boundary. This use of MAXALIGN is wasted space anyway; the arrays only need SHORTALIGN, which the code maintains without assistance (so long as each xlrec structure needs at least SHORTALIGN, which seems inevitable). I've just removed the explicit alignment bumps. This behavior dates to the initial SP-GiST commit. Since every palloc chunk has MAXALIGN'd size under the hood, the excess read cannot cause a SIGSEGV or other concrete bad outcome. Therefore, I propose to change 9.4 only. * GinFormTuple() leaves pad bytes uninitialized When it repalloc()s an IndexTuple, this function does not zero the new space. The increase has up to two regions of alignment padding, one byte after the null category (if any) and up to six bytes at the end. This is not, to my knowledge, a concrete problem. However, I've observed no other place in the system where we send uninitialized data to PageAddItem(). This looks like an oversight, and we should clear affected memory to bring consistency with the other core bufpage consumers. This behavior goes back to 8.4 or earlier. Since it's a mere point of hygiene, I intend this for 9.4 only. * Uses of a NULL-terminated string as a Name datum A Name is expected to have NAMEDATALEN bytes. Presenting a shorter cstring as a Name causes reading past the end of the string's allocation. Switch to the usual idiom. New in 9.3 (765cbfdc9263bf7c90b9d1f1044c6950b8b7088c, 3855968f328918b6cd1401dd11d109d471a54d40). * HaveVirtualXIDsDelayingChkpt() wrongly assumes a terminated array This function looks for a !VirtualTransactionIdIsValid() terminator, but GetVirtualXIDsDelayingChkpt() does not add one. Patch makes the function's looping logic more like its 9.2 version; I cannot find discussion of or discern a benefit of that aspect of the change. It also reverts the addition of an unused variable, presumably a a debugging relic, by the same commit. New in 9.3 (f21bb9cfb5646e1793dcc9c0ea697bab99afa523). * Passing oidvector by value oidvector ends with a flexible array, so this is almost always wrong. New in 9.3 (7ac5760fa283bc090c25e4ea495a0d2bb41db7b5). * hba.c tokenize_file can read backward past the beginning of a stack variable This arises when a file like pg_hba.conf contains an empty line. New in 9.3 (7f49a67f954db3e92fd96963169fb8302959576e). * json parser can read one byte past the datum end I swapped order of tests like "if (has_some_property(*p) && p < end)". Perhaps this was intended as a micro-optimization, putting the more-selective check first. If that is important, we might arrange to have a known byte after the end to make it safe. New in 9.3 (a570c98d7fa0841f17bbf51d62d02d9e493c7fcc). Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c index 5f6bcdd..1199916 100644 --- a/src/backend/access/spgist/spgdoinsert.c +++ b/src/backend/access/spgist/spgdoinsert.c @@ -535,9 +535,9 @@ moveLeafs(Relation index, SpGistState *state, { XLogRecPtr recptr; - ACCEPT_RDATA_DATA(&xlrec, MAXALIGN(sizeof(xlrec)), 0); - ACCEPT_RDATA_DATA(toDelete, MAXALIGN(sizeof(OffsetNumber) * nDelete), 1); - ACCEPT_RDATA_DATA(toInsert, MAXALIGN(sizeof(OffsetNumber) * nInsert), 2); + ACCEPT_RDATA_DATA(&xlrec, sizeof(xlrec), 0); + ACCEPT_RDATA_DATA(toDelete, sizeof(OffsetNumber) * nDelete, 1); + ACCEPT_RDATA_DATA(toInsert, sizeof(OffsetNumber) * nInsert, 2); ACCEPT_RDATA_DATA(leafdata, leafptr - leafdata, 3); ACCEPT_RDATA_BUFFER(current->buffer, 4); ACCEPT_RDATA_BUFFER(nbuf, 5); @@ -1118,7 +1118,7 @@ doPickSplit(Relation index, SpGistState *state, leafdata = leafptr = (char *) palloc(totalLeafSizes); - ACCEPT_RDATA_DATA(&xlrec, MAXALIGN(sizeof(xlrec)), 0); + ACCEPT_RDATA_DATA(&xlrec, sizeof(xlrec), 0); ACCEPT_RDATA_DATA(innerTuple, innerTuple->size, 1); nRdata = 2; @@ -1154,7 +1154,7 @@ doPickSplit(Relation index, SpGistState *state, { xlrec.nDelete = nToDelete; ACCEPT_RDATA_DATA(toDelete, - MAXALIGN(sizeof(OffsetNumber) * nToDelete), + sizeof(OffsetNumber) * nToDelete, nRdata); nRdata++; ACCEPT_RDATA_BUFFER(current->buffer, nRdata); @@ -1254,11 +1254,11 @@ doPickSplit(Relation index, SpGistState *state, xlrec.nInsert = nToInsert; ACCEPT_RDATA_DATA(toInsert, - MAXALIGN(sizeof(OffsetNumber) * nToInsert), + sizeof(OffsetNumber) * nToInsert, nRdata); nRdata++; ACCEPT_RDATA_DATA(leafPageSelect, - MAXALIGN(sizeof(uint8) * nToInsert), + sizeof(uint8) * nToInsert, nRdata); nRdata++; ACCEPT_RDATA_DATA(leafdata, leafptr - leafdata, nRdata); diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c index 3f5556f..dbc19e7 100644 --- a/src/backend/access/spgist/spgxlog.c +++ b/src/backend/access/spgist/spgxlog.c @@ -217,11 +217,11 @@ spgRedoMoveLeafs(XLogRecPtr lsn, XLogRecord *record) nInsert = xldata->replaceDead ? 1 : xldata->nMoves + 1; - ptr += MAXALIGN(sizeof(spgxlogMoveLeafs)); + ptr += sizeof(spgxlogMoveLeafs); toDelete = (OffsetNumber *) ptr; - ptr += MAXALIGN(sizeof(OffsetNumber) * xldata->nMoves); + ptr += sizeof(OffsetNumber) * xldata->nMoves; toInsert = (OffsetNumber *) ptr; - ptr += MAXALIGN(sizeof(OffsetNumber) * nInsert); + ptr += sizeof(OffsetNumber) * nInsert; /* now ptr points to the list of leaf tuples */ @@ -602,15 +602,15 @@ spgRedoPickSplit(XLogRecPtr lsn, XLogRecord *record) fillFakeState(&state, xldata->stateSrc); - ptr += MAXALIGN(sizeof(spgxlogPickSplit)); + ptr += sizeof(spgxlogPickSplit); innerTuple = (SpGistInnerTuple) ptr; ptr += innerTuple->size; toDelete = (OffsetNumber *) ptr; - ptr += MAXALIGN(sizeof(OffsetNumber) * xldata->nDelete); + ptr += sizeof(OffsetNumber) * xldata->nDelete; toInsert = (OffsetNumber *) ptr; - ptr += MAXALIGN(sizeof(OffsetNumber) * xldata->nInsert); + ptr += sizeof(OffsetNumber) * xldata->nInsert; leafPageSelect = (uint8 *) ptr; - ptr += MAXALIGN(sizeof(uint8) * xldata->nInsert); + ptr += sizeof(uint8) * xldata->nInsert; /* now ptr points to the list of leaf tuples */
*** a/src/backend/access/gin/ginentrypage.c --- b/src/backend/access/gin/ginentrypage.c *************** *** 112,117 **** GinFormTuple(GinState *ginstate, --- 112,123 ---- if (newsize != IndexTupleSize(itup)) { itup = repalloc(itup, newsize); + /* + * PostgreSQL 9.3 and earlier did not clear this new space, so we + * might find uninitialized padding when reading tuples from disk. + */ + memset((char *) itup + IndexTupleSize(itup), + 0, newsize - IndexTupleSize(itup)); /* set new size in tuple header */ itup->t_info &= ~INDEX_SIZE_MASK;
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index 178c979..bb6c1a4 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -168,6 +168,7 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name) Datum *values; bool *nulls; bool *replaces; + NameData nameattrdata; oldtup = SearchSysCache1(oidCacheId, ObjectIdGetDatum(objectId)); if (!HeapTupleIsValid(oldtup)) @@ -273,7 +274,8 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name) values = palloc0(RelationGetNumberOfAttributes(rel) * sizeof(Datum)); nulls = palloc0(RelationGetNumberOfAttributes(rel) * sizeof(bool)); replaces = palloc0(RelationGetNumberOfAttributes(rel) * sizeof(bool)); - values[Anum_name - 1] = PointerGetDatum(new_name); + namestrcpy(&nameattrdata, new_name); + values[Anum_name - 1] = NameGetDatum(&nameattrdata); replaces[Anum_name - 1] = true; newtup = heap_modify_tuple(oldtup, RelationGetDescr(rel), values, nulls, replaces); diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c index a0f97e4..328e2a8 100644 --- a/src/backend/commands/event_trigger.c +++ b/src/backend/commands/event_trigger.c @@ -302,6 +302,8 @@ insert_event_trigger_tuple(char *trigname, char *eventname, Oid evtOwner, HeapTuple tuple; Datum values[Natts_pg_trigger]; bool nulls[Natts_pg_trigger]; + NameData evtnamedata, + evteventdata; ObjectAddress myself, referenced; @@ -310,8 +312,10 @@ insert_event_trigger_tuple(char *trigname, char *eventname, Oid evtOwner, /* Build the new pg_trigger tuple. */ memset(nulls, false, sizeof(nulls)); - values[Anum_pg_event_trigger_evtname - 1] = NameGetDatum(trigname); - values[Anum_pg_event_trigger_evtevent - 1] = NameGetDatum(eventname); + namestrcpy(&evtnamedata, trigname); + values[Anum_pg_event_trigger_evtname - 1] = NameGetDatum(&evtnamedata); + namestrcpy(&evteventdata, eventname); + values[Anum_pg_event_trigger_evtevent - 1] = NameGetDatum(&evteventdata); values[Anum_pg_event_trigger_evtowner - 1] = ObjectIdGetDatum(evtOwner); values[Anum_pg_event_trigger_evtfoid - 1] = ObjectIdGetDatum(funcoid); values[Anum_pg_event_trigger_evtenabled - 1] =
*** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *************** *** 6984,6995 **** CreateCheckPoint(int flags) vxids = GetVirtualXIDsDelayingChkpt(&nvxids); if (nvxids > 0) { - uint32 nwaits = 0; - do { pg_usleep(10000L); /* wait for 10 msec */ - nwaits++; } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids)); } pfree(vxids); --- 6984,6992 ---- *** a/src/backend/storage/ipc/procarray.c --- b/src/backend/storage/ipc/procarray.c *************** *** 1849,1880 **** HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) LWLockAcquire(ProcArrayLock, LW_SHARED); ! while (VirtualTransactionIdIsValid(*vxids)) { ! for (index = 0; index < arrayP->numProcs; index++) { ! int pgprocno = arrayP->pgprocnos[index]; ! volatile PGPROC *proc = &allProcs[pgprocno]; ! volatile PGXACT *pgxact = &allPgXact[pgprocno]; ! VirtualTransactionId vxid; ! GET_VXID_FROM_PGPROC(vxid, *proc); ! if (VirtualTransactionIdIsValid(vxid)) { ! if (VirtualTransactionIdEquals(vxid, *vxids) && ! pgxact->delayChkpt) { result = true; break; } } } - - if (result) - break; - - /* The virtual transaction is gone now, wait for the next one */ - vxids++; } LWLockRelease(ProcArrayLock); --- 1849,1878 ---- LWLockAcquire(ProcArrayLock, LW_SHARED); ! for (index = 0; index < arrayP->numProcs; index++) { ! int pgprocno = arrayP->pgprocnos[index]; ! volatile PGPROC *proc = &allProcs[pgprocno]; ! volatile PGXACT *pgxact = &allPgXact[pgprocno]; ! VirtualTransactionId vxid; ! ! GET_VXID_FROM_PGPROC(vxid, *proc); ! ! if (pgxact->delayChkpt && VirtualTransactionIdIsValid(vxid)) { ! int i; ! for (i = 0; i < nvxids; i++) { ! if (VirtualTransactionIdEquals(vxid, vxids[i])) { result = true; break; } } + if (result) + break; } } LWLockRelease(ProcArrayLock);
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index 178c979..47c8869 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -230,7 +230,7 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name) Form_pg_proc proc = (Form_pg_proc) GETSTRUCT(oldtup); IsThereFunctionInNamespace(new_name, proc->pronargs, - proc->proargtypes, proc->pronamespace); + &proc->proargtypes, proc->pronamespace); } else if (classId == CollationRelationId) { @@ -609,7 +609,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) Form_pg_proc proc = (Form_pg_proc) GETSTRUCT(tup); IsThereFunctionInNamespace(NameStr(proc->proname), proc->pronargs, - proc->proargtypes, nspOid); + &proc->proargtypes, nspOid); } else if (classId == CollationRelationId) { diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 38187a8..c776758 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -1626,18 +1626,18 @@ DropCastById(Oid castOid) */ void IsThereFunctionInNamespace(const char *proname, int pronargs, - oidvector proargtypes, Oid nspOid) + oidvector *proargtypes, Oid nspOid) { /* check for duplicate name (more friendly than unique-index failure) */ if (SearchSysCacheExists3(PROCNAMEARGSNSP, CStringGetDatum(proname), - PointerGetDatum(&proargtypes), + PointerGetDatum(proargtypes), ObjectIdGetDatum(nspOid))) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_FUNCTION), errmsg("function %s already exists in schema \"%s\"", funcname_signature_string(proname, pronargs, - NIL, proargtypes.values), + NIL, proargtypes->values), get_namespace_name(nspOid)))); } diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index 01d165f..fa9f41f 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -50,7 +50,7 @@ extern Oid AlterFunction(AlterFunctionStmt *stmt); extern Oid CreateCast(CreateCastStmt *stmt); extern void DropCastById(Oid castOid); extern void IsThereFunctionInNamespace(const char *proname, int pronargs, - oidvector proargtypes, Oid nspOid); + oidvector *proargtypes, Oid nspOid); extern void ExecuteDoStmt(DoStmt *stmt); extern Oid get_cast_oid(Oid sourcetypeid, Oid targettypeid, bool missing_ok);
*** a/src/backend/libpq/hba.c --- b/src/backend/libpq/hba.c *************** *** 411,419 **** tokenize_file(const char *filename, FILE *file, line_number, filename))); /* Strip trailing linebreak from rawline */ ! while (rawline[strlen(rawline) - 1] == '\n' || ! rawline[strlen(rawline) - 1] == '\r') ! rawline[strlen(rawline) - 1] = '\0'; lineptr = rawline; while (strlen(lineptr) > 0) --- 411,419 ---- line_number, filename))); /* Strip trailing linebreak from rawline */ ! lineptr = rawline + strlen(rawline) - 1; ! while (lineptr >= rawline && (*lineptr == '\n' || *lineptr == '\r')) ! *lineptr-- = '\0'; lineptr = rawline; while (strlen(lineptr) > 0)
*** a/src/backend/utils/adt/json.c --- b/src/backend/utils/adt/json.c *************** *** 598,604 **** json_lex(JsonLexContext *lex) * the whole word as an unexpected token, rather than just * some unintuitive prefix thereof. */ ! for (p = s; JSON_ALPHANUMERIC_CHAR(*p) && p - s < lex->input_length - len; p++) /* skip */ ; /* --- 598,604 ---- * the whole word as an unexpected token, rather than just * some unintuitive prefix thereof. */ ! for (p = s; p - s < lex->input_length - len && JSON_ALPHANUMERIC_CHAR(*p); p++) /* skip */ ; /* *************** *** 650,665 **** json_lex_string(JsonLexContext *lex) if (lex->strval != NULL) resetStringInfo(lex->strval); len = lex->token_start - lex->input; ! len++; ! for (s = lex->token_start + 1; *s != '"'; s++, len++) { /* Premature end of the string. */ if (len >= lex->input_length) { lex->token_terminator = s; report_invalid_token(lex); } else if ((unsigned char) *s < 32) { /* Per RFC4627, these characters MUST be escaped. */ --- 650,670 ---- if (lex->strval != NULL) resetStringInfo(lex->strval); + Assert(lex->input_length > 0); + s = lex->token_start; len = lex->token_start - lex->input; ! for (;;) { + s++; + len++; /* Premature end of the string. */ if (len >= lex->input_length) { lex->token_terminator = s; report_invalid_token(lex); } + else if (*s == '"') + break; else if ((unsigned char) *s < 32) { /* Per RFC4627, these characters MUST be escaped. */ *************** *** 843,849 **** json_lex_number(JsonLexContext *lex, char *s) { s++; len++; ! } while (*s >= '0' && *s <= '9' && len < lex->input_length); } else error = true; --- 848,854 ---- { s++; len++; ! } while (len < lex->input_length && *s >= '0' && *s <= '9'); } else error = true; *************** *** 861,867 **** json_lex_number(JsonLexContext *lex, char *s) { s++; len++; ! } while (*s >= '0' && *s <= '9' && len < lex->input_length); } } --- 866,872 ---- { s++; len++; ! } while (len < lex->input_length && *s >= '0' && *s <= '9'); } } *************** *** 892,898 **** json_lex_number(JsonLexContext *lex, char *s) * here should be considered part of the token for error-reporting * purposes. */ ! for (p = s; JSON_ALPHANUMERIC_CHAR(*p) && len < lex->input_length; p++, len++) error = true; lex->prev_token_terminator = lex->token_terminator; lex->token_terminator = p; --- 897,903 ---- * here should be considered part of the token for error-reporting * purposes. */ ! for (p = s; len < lex->input_length && JSON_ALPHANUMERIC_CHAR(*p); p++, len++) error = true; lex->prev_token_terminator = lex->token_terminator; lex->token_terminator = p; *************** *** 1060,1067 **** report_json_context(JsonLexContext *lex) line_number = 1; for (;;) { ! /* Always advance over newlines (context_end test is just paranoia) */ ! if (*context_start == '\n' && context_start < context_end) { context_start++; line_start = context_start; --- 1065,1072 ---- line_number = 1; for (;;) { ! /* Always advance over newlines */ ! if (context_start < context_end && *context_start == '\n') { context_start++; line_start = context_start;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers