Remove FATAL from pg_lzdecompress

Started by Zdenek Kotalaover 18 years ago6 messagespatches
Jump to latest
#1Zdenek Kotala
Zdenek.Kotala@Sun.COM

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

Attachments:

lztoast.difftext/x-patch; name=lztoast.diffDownload+17-13
#2Bruce Momjian
bruce@momjian.us
In reply to: Zdenek Kotala (#1)
Re: Remove FATAL from pg_lzdecompress

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. +

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Zdenek Kotala (#1)
Re: Remove FATAL from pg_lzdecompress

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

#4Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Tom Lane (#3)
Re: Remove FATAL from pg_lzdecompress

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

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Zdenek Kotala (#4)
Re: Remove FATAL from pg_lzdecompress

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

#6Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Tom Lane (#5)
Re: Remove FATAL from pg_lzdecompress

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