Stephen Frost <sfr...@snowman.net> writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> There are at least two places in inv_api.c where we have
>> "Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
>> into a local variable of size LOBLKSIZE, so that the only thing standing
>> between us and a stack-smash security issue that's trivially exploitable
>> in production builds is that on-disk data conforms to our expectation
>> about LOBLKSIZE. I think it's definitely worth promoting these checks
>> to regular runtime-if-test-and-elog.
> Agreed. Promoting that to a run-time check seems well worth it to me.
Here's a draft patch for this. Barring objections I'll commit the whole
thing to HEAD, and the inv_api.c changes to the back branches as well.
regards, tom lane
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d675560..a61878e 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 49,54 ****
--- 49,55 ----
#include "storage/bufmgr.h"
#include "storage/fd.h"
#include "storage/ipc.h"
+ #include "storage/large_object.h"
#include "storage/latch.h"
#include "storage/pmsignal.h"
#include "storage/predicate.h"
*************** WriteControlFile(void)
*** 4352,4357 ****
--- 4353,4359 ----
ControlFile->indexMaxKeys = INDEX_MAX_KEYS;
ControlFile->toast_max_chunk_size = TOAST_MAX_CHUNK_SIZE;
+ ControlFile->loblksize = LOBLKSIZE;
#ifdef HAVE_INT64_TIMESTAMP
ControlFile->enableIntTimes = true;
*************** ReadControlFile(void)
*** 4545,4550 ****
--- 4547,4559 ----
" but the server was compiled with TOAST_MAX_CHUNK_SIZE %d.",
ControlFile->toast_max_chunk_size, (int) TOAST_MAX_CHUNK_SIZE),
errhint("It looks like you need to recompile or initdb.")));
+ if (ControlFile->loblksize != LOBLKSIZE)
+ ereport(FATAL,
+ (errmsg("database files are incompatible with server"),
+ errdetail("The database cluster was initialized with LOBLKSIZE %d,"
+ " but the server was compiled with LOBLKSIZE %d.",
+ ControlFile->loblksize, (int) LOBLKSIZE),
+ errhint("It looks like you need to recompile or initdb.")));
#ifdef HAVE_INT64_TIMESTAMP
if (ControlFile->enableIntTimes != true)
diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index 57ec1c2..0918142 100644
*** a/src/backend/storage/large_object/inv_api.c
--- b/src/backend/storage/large_object/inv_api.c
*************** myLargeObjectExists(Oid loid, Snapshot s
*** 173,185 ****
}
! static int32
! getbytealen(bytea *data)
{
! Assert(!VARATT_IS_EXTENDED(data));
! if (VARSIZE(data) < VARHDRSZ)
! elog(ERROR, "invalid VARSIZE(data)");
! return (VARSIZE(data) - VARHDRSZ);
}
--- 173,210 ----
}
! /*
! * Extract data field from a pg_largeobject tuple, detoasting if needed
! * and verifying that the length is sane. Returns data pointer (a bytea *),
! * data length, and an indication of whether to pfree the data pointer.
! */
! static void
! getdatafield(Form_pg_largeobject tuple,
! bytea **pdatafield,
! int *plen,
! bool *pfreeit)
{
! bytea *datafield;
! int len;
! bool freeit;
!
! datafield = &(tuple->data); /* see note at top of file */
! freeit = false;
! if (VARATT_IS_EXTENDED(datafield))
! {
! datafield = (bytea *)
! heap_tuple_untoast_attr((struct varlena *) datafield);
! freeit = true;
! }
! len = VARSIZE(datafield) - VARHDRSZ;
! if (len < 0 || len > LOBLKSIZE)
! ereport(ERROR,
! (errcode(ERRCODE_DATA_CORRUPTED),
! errmsg("pg_largeobject entry for OID %u, page %d has invalid data field size %d",
! tuple->loid, tuple->pageno, len)));
! *pdatafield = datafield;
! *plen = len;
! *pfreeit = freeit;
}
*************** inv_getsize(LargeObjectDesc *obj_desc)
*** 366,385 ****
{
Form_pg_largeobject data;
bytea *datafield;
bool pfreeit;
if (HeapTupleHasNulls(tuple)) /* paranoia */
elog(ERROR, "null field found in pg_largeobject");
data = (Form_pg_largeobject) GETSTRUCT(tuple);
! datafield = &(data->data); /* see note at top of file */
! pfreeit = false;
! if (VARATT_IS_EXTENDED(datafield))
! {
! datafield = (bytea *)
! heap_tuple_untoast_attr((struct varlena *) datafield);
! pfreeit = true;
! }
! lastbyte = (uint64) data->pageno * LOBLKSIZE + getbytealen(datafield);
if (pfreeit)
pfree(datafield);
}
--- 391,404 ----
{
Form_pg_largeobject data;
bytea *datafield;
+ int len;
bool pfreeit;
if (HeapTupleHasNulls(tuple)) /* paranoia */
elog(ERROR, "null field found in pg_largeobject");
data = (Form_pg_largeobject) GETSTRUCT(tuple);
! getdatafield(data, &datafield, &len, &pfreeit);
! lastbyte = (uint64) data->pageno * LOBLKSIZE + len;
if (pfreeit)
pfree(datafield);
}
*************** inv_read(LargeObjectDesc *obj_desc, char
*** 506,520 ****
off = (int) (obj_desc->offset - pageoff);
Assert(off >= 0 && off < LOBLKSIZE);
! datafield = &(data->data); /* see note at top of file */
! pfreeit = false;
! if (VARATT_IS_EXTENDED(datafield))
! {
! datafield = (bytea *)
! heap_tuple_untoast_attr((struct varlena *) datafield);
! pfreeit = true;
! }
! len = getbytealen(datafield);
if (len > off)
{
n = len - off;
--- 525,531 ----
off = (int) (obj_desc->offset - pageoff);
Assert(off >= 0 && off < LOBLKSIZE);
! getdatafield(data, &datafield, &len, &pfreeit);
if (len > off)
{
n = len - off;
*************** inv_write(LargeObjectDesc *obj_desc, con
*** 630,645 ****
*
* First, load old data into workbuf
*/
! datafield = &(olddata->data); /* see note at top of file */
! pfreeit = false;
! if (VARATT_IS_EXTENDED(datafield))
! {
! datafield = (bytea *)
! heap_tuple_untoast_attr((struct varlena *) datafield);
! pfreeit = true;
! }
! len = getbytealen(datafield);
! Assert(len <= LOBLKSIZE);
memcpy(workb, VARDATA(datafield), len);
if (pfreeit)
pfree(datafield);
--- 641,647 ----
*
* First, load old data into workbuf
*/
! getdatafield(olddata, &datafield, &len, &pfreeit);
memcpy(workb, VARDATA(datafield), len);
if (pfreeit)
pfree(datafield);
*************** inv_truncate(LargeObjectDesc *obj_desc,
*** 815,833 ****
if (olddata != NULL && olddata->pageno == pageno)
{
/* First, load old data into workbuf */
! bytea *datafield = &(olddata->data); /* see note at top of
! * file */
! bool pfreeit = false;
int pagelen;
! if (VARATT_IS_EXTENDED(datafield))
! {
! datafield = (bytea *)
! heap_tuple_untoast_attr((struct varlena *) datafield);
! pfreeit = true;
! }
! pagelen = getbytealen(datafield);
! Assert(pagelen <= LOBLKSIZE);
memcpy(workb, VARDATA(datafield), pagelen);
if (pfreeit)
pfree(datafield);
--- 817,827 ----
if (olddata != NULL && olddata->pageno == pageno)
{
/* First, load old data into workbuf */
! bytea *datafield;
int pagelen;
+ bool pfreeit;
! getdatafield(olddata, &datafield, &pagelen, &pfreeit);
memcpy(workb, VARDATA(datafield), pagelen);
if (pfreeit)
pfree(datafield);
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 24e2116..f815024 100644
*** a/src/bin/pg_controldata/pg_controldata.c
--- b/src/bin/pg_controldata/pg_controldata.c
*************** main(int argc, char *argv[])
*** 287,292 ****
--- 287,294 ----
ControlFile.indexMaxKeys);
printf(_("Maximum size of a TOAST chunk: %u\n"),
ControlFile.toast_max_chunk_size);
+ printf(_("Size of a large-object chunk: %u\n"),
+ ControlFile.loblksize);
printf(_("Date/time type storage: %s\n"),
(ControlFile.enableIntTimes ? _("64-bit integers") : _("floating-point numbers")));
printf(_("Float4 argument passing: %s\n"),
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index d11280e..915a1ed 100644
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
***************
*** 52,57 ****
--- 52,58 ----
#include "catalog/catversion.h"
#include "catalog/pg_control.h"
#include "common/fe_memutils.h"
+ #include "storage/large_object.h"
#include "pg_getopt.h"
*************** GuessControlValues(void)
*** 536,541 ****
--- 537,543 ----
ControlFile.nameDataLen = NAMEDATALEN;
ControlFile.indexMaxKeys = INDEX_MAX_KEYS;
ControlFile.toast_max_chunk_size = TOAST_MAX_CHUNK_SIZE;
+ ControlFile.loblksize = LOBLKSIZE;
#ifdef HAVE_INT64_TIMESTAMP
ControlFile.enableIntTimes = true;
#else
*************** PrintControlValues(bool guessed)
*** 620,625 ****
--- 622,629 ----
ControlFile.indexMaxKeys);
printf(_("Maximum size of a TOAST chunk: %u\n"),
ControlFile.toast_max_chunk_size);
+ printf(_("Size of a large-object chunk: %u\n"),
+ ControlFile.loblksize);
printf(_("Date/time type storage: %s\n"),
(ControlFile.enableIntTimes ? _("64-bit integers") : _("floating-point numbers")));
printf(_("Float4 argument passing: %s\n"),
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index 4579eb6..ba79d25 100644
*** a/src/include/catalog/pg_control.h
--- b/src/include/catalog/pg_control.h
***************
*** 21,27 ****
/* Version identifier for this pg_control format */
! #define PG_CONTROL_VERSION 941
/*
* Body of CheckPoint XLOG records. This is declared here because we keep
--- 21,27 ----
/* Version identifier for this pg_control format */
! #define PG_CONTROL_VERSION 942
/*
* Body of CheckPoint XLOG records. This is declared here because we keep
*************** typedef struct ControlFileData
*** 207,212 ****
--- 207,213 ----
uint32 indexMaxKeys; /* max number of columns in an index */
uint32 toast_max_chunk_size; /* chunk size in TOAST tables */
+ uint32 loblksize; /* chunk size in pg_largeobject */
/* flag indicating internal format of timestamp, interval, time */
bool enableIntTimes; /* int64 storage enabled? */
diff --git a/src/include/storage/large_object.h b/src/include/storage/large_object.h
index 0d81a4b..30438a9 100644
*** a/src/include/storage/large_object.h
--- b/src/include/storage/large_object.h
*************** typedef struct LargeObjectDesc
*** 66,71 ****
--- 66,73 ----
* Also, it seems to be a smart move to make the page size be a power of 2,
* since clients will often be written to send data in power-of-2 blocks.
* This avoids unnecessary tuple updates caused by partial-page writes.
+ *
+ * NB: Changing LOBLKSIZE requires an initdb.
*/
#define LOBLKSIZE (BLCKSZ / 4)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
