Thread: Remove FATAL from pg_lzdecompress
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");
/*
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Zdenek Kotala wrote:
>
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> I attach patch which adds boundaries check and memory overwriting
> protection when compressed data are corrupted.
Applied with revisions --- it appeared to me that it got the corner case
wrong where we find a tag just at the end of the input but there's no
room for the output. We'd fall out of the loop and then the error
test would think all is well.
> 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 still unconvinced that that's worth any added complexity or
slowdown.
regards, tom lane
Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> I attach patch which adds boundaries check and memory overwriting
>> protection when compressed data are corrupted.
>
> Applied with revisions --- it appeared to me that it got the corner case
> wrong where we find a tag just at the end of the input but there's no
> room for the output. We'd fall out of the loop and then the error
> test would think all is well.
Good point. Is there plan to applied also on other branch? I think it is
useful fix for production release as well. Especially when I want to
check all tuples and report tid of corrupted tuples, I'm not able handle
FATAL exception.
Thanks Zdenek
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>> I attach patch which adds boundaries check and memory overwriting
>>> protection when compressed data are corrupted.
> Good point. Is there plan to applied also on other branch?
I wasn't planning to back-patch it. Given the lack of field reports
of compressed-data problems, it seemed to me that the risk of breaking
something was larger than the chance of helping someone. We could
reconsider this after the code has been in HEAD awhile, perhaps.
regards, tom lane
Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>>> I attach patch which adds boundaries check and memory overwriting
>>>> protection when compressed data are corrupted.
>
>> Good point. Is there plan to applied also on other branch?
>
> I wasn't planning to back-patch it. Given the lack of field reports
> of compressed-data problems, it seemed to me that the risk of breaking
> something was larger than the chance of helping someone. We could
> reconsider this after the code has been in HEAD awhile, perhaps.
Tom,
one of our customer with 3TB table it uses now in production (8.2) awhile (2
weeks) and it works pretty well. He had a corrupted data in TOASTed table and
now his system is stable without random crashes. I plan to use this patch in
official Solaris build, but I prefer do not have differences between main stream
and solaris binaries. Would be possible to backported this patch?
Thanks Zdenek