Re: pg_control is missing a field for LOBLKSIZE - Mailing list pgsql-hackers

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@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)