Remove FATAL from pg_lzdecompress - Mailing list pgsql-patches

From Zdenek Kotala
Subject Remove FATAL from pg_lzdecompress
Date
Msg-id 47C86D9E.2000909@sun.com
Whole thread Raw
Responses Re: Remove FATAL from pg_lzdecompress  (Bruce Momjian <bruce@momjian.us>)
Re: Remove FATAL from pg_lzdecompress  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-patches
I attach patch which adds boundaries check and memory overwriting
protection when compressed data are corrupted.

Current behavior let code overwrite a memory and after that check if
unpacked size is same as expected value. In this case elog execution
fails (at least on Solaris - malloc has corrupted structures) and no
message appears in a log file.

I did not add any extra information into the message. Reasonable
solution seems to be use errcontext how was recommended by Alvaro. But I
'm not sure if printtup is good place for it, because pg_detoast is
called from many places. However, is can be solved in separate patch.

I'm also think that this modification should be backported to other
version too.

        Thanks Zdenek
Index: src/backend/utils/adt/pg_lzcompress.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/utils/adt/pg_lzcompress.c,v
retrieving revision 1.29
diff -c -r1.29 pg_lzcompress.c
*** src/backend/utils/adt/pg_lzcompress.c    1 Jan 2008 19:45:52 -0000    1.29
--- src/backend/utils/adt/pg_lzcompress.c    29 Feb 2008 19:07:50 -0000
***************
*** 633,638 ****
--- 633,639 ----
  {
      const unsigned char *dp;
      const unsigned char *dend;
+     const unsigned char *destend;
      unsigned char *bp;
      unsigned char ctrl;
      int32        ctrlc;
***************
*** 643,656 ****
      dp = ((const unsigned char *) source) + sizeof(PGLZ_Header);
      dend = ((const unsigned char *) source) + VARSIZE(source);
      bp = (unsigned char *) dest;

!     while (dp < dend)
      {
          /*
           * Read one control byte and process the next 8 items.
           */
          ctrl = *dp++;
!         for (ctrlc = 0; ctrlc < 8 && dp < dend; ctrlc++)
          {
              if (ctrl & 1)
              {
--- 644,658 ----
      dp = ((const unsigned char *) source) + sizeof(PGLZ_Header);
      dend = ((const unsigned char *) source) + VARSIZE(source);
      bp = (unsigned char *) dest;
+     destend = ((const unsigned char *)dest) + source->rawsize;

!     while (dp < dend && bp < destend)
      {
          /*
           * Read one control byte and process the next 8 items.
           */
          ctrl = *dp++;
!         for (ctrlc = 0; ctrlc < 8 && dp < dend && bp < destend; ctrlc++)
          {
              if (ctrl & 1)
              {
***************
*** 673,678 ****
--- 675,682 ----
                   * memcpy() here, because the copied areas could overlap
                   * extremely!
                   */
+                 if( bp+len > destend) /* data are corrupted, do not continue */
+                     break;
                  while (len--)
                  {
                      *bp = bp[-off];
***************
*** 696,708 ****
      }

      /*
!      * Check we decompressed the right amount, else die.  This is a FATAL
!      * condition if we tromped on more memory than expected (we assume we have
!      * not tromped on shared memory, though, so need not PANIC).
       */
!     destsize = (char *) bp - dest;
!     if (destsize != source->rawsize)
!         elog(destsize > source->rawsize ? FATAL : ERROR,
               "compressed data is corrupt");

      /*
--- 700,709 ----
      }

      /*
!      * Check we decompressed the right amount.
       */
!     if (bp != destend || dp != dend)
!         elog(ERROR,
               "compressed data is corrupt");

      /*

pgsql-patches by date:

Previous
From: Tom Lane
Date:
Subject: Re: DTrace probe patch for OS X Leopard
Next
From: Alvaro Herrera
Date:
Subject: Re: DTrace probe patch for OS X Leopard