From: Flavien GUEDEZ <flav.pg@oopacity.net>
To: pgsql-bugs@lists.postgresql.org
Subject: Re: Insufficient memory access checks in pglz_decompress
Date: Wed, 18 Oct 2023 18:46:56 +0200
Message-ID: <bb74b9f4-65c1-4d89-af20-7de623948e36@oopacity.net> (raw)
In-Reply-To: <3116807.1697642067@sss.pgh.pa.us>
References: <01fc0593-e31e-463d-902c-dd43174acee2@oopacity.net>
<3116807.1697642067@sss.pgh.pa.us>
Thanks for your feedback, you are definitely right, I did not notice
that (dp - off) was staying the same in the while loop.
Here is another much smaller patch.
Flavien
Le 18/10/2023 à 17:14, Tom Lane a écrit :
> Flavien GUEDEZ <flav.pg@oopacity.net> writes:
>> After some investigations about very corrupted toast data in one
>> postgres instance, I found that the pglz_decompress function (in
>> common/pg_lzcompress.c) does not check correctly where it copies data
>> from using memcpy(), which could result in segfault.
>> In this function, there are other checks to ensure that we do not copy
>> after the destination end, but not if we copy data from "before the
>> beginning".
> Hmm, would it not be better to add this check to the existing "Check for
> corrupt data" a bit further up? Then you'd only need one instance of
> the test, and only need to do it once per tag (note the comment pointing
> out that dp - off stays the same), and overall it'd be less surprising IMO.
>
> regards, tom lane
Attachments:
[text/x-patch] v2-pglz_decompress_check_for_corrupted.patch (549B, ../bb74b9f4-65c1-4d89-af20-7de623948e36@oopacity.net/2-v2-pglz_decompress_check_for_corrupted.patch)
download | inline diff:--- a/src/common/pg_lzcompress.c 2023-10-18 12:34:30.616000000 +0000+++ b/src/common/pg_lzcompress.c 2023-10-18 16:42:17.856000000 +0000@@ -738,8 +738,10 @@
* source, or if we obtained off = 0, we have problems. (We
* must check this, else we risk an infinite loop below in the
* face of corrupt data.)
+ * Also check if we fell before the beginning of the+ * destination.
*/
- if (unlikely(sp > srcend || off == 0))+ if (unlikely(sp > srcend || off == 0 || (dp - off) < dest))
return -1;
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: flav.pg@oopacity.net, pgsql-bugs@lists.postgresql.org
Subject: Re: Insufficient memory access checks in pglz_decompress
In-Reply-To: <bb74b9f4-65c1-4d89-af20-7de623948e36@oopacity.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox