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
