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