Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qt9hH-00B4II-2h for pgsql-bugs@arkaria.postgresql.org; Wed, 18 Oct 2023 16:47:03 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1qt9hE-006Uw8-Vu for pgsql-bugs@arkaria.postgresql.org; Wed, 18 Oct 2023 16:47:01 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qt9hE-006Uux-NN for pgsql-bugs@lists.postgresql.org; Wed, 18 Oct 2023 16:47:01 +0000 Received: from blue.particules.net ([87.98.130.204] helo=mail.particules.net) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qt9hB-001OTO-AN for pgsql-bugs@lists.postgresql.org; Wed, 18 Oct 2023 16:47:00 +0000 Received: from [192.168.2.100] (unknown [192.168.2.100]) by mail.particules.net (Postfix) with ESMTP id 1D1F1ACAA for ; Wed, 18 Oct 2023 16:46:57 +0000 (UTC) Content-Type: multipart/mixed; boundary="------------nSk4lodlxKe3x2k9KP8uzdhi" Message-ID: Date: Wed, 18 Oct 2023 18:46:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Insufficient memory access checks in pglz_decompress Content-Language: fr, en-US To: pgsql-bugs@lists.postgresql.org References: <01fc0593-e31e-463d-902c-dd43174acee2@oopacity.net> <3116807.1697642067@sss.pgh.pa.us> From: Flavien GUEDEZ Autocrypt: addr=flavien@guedez.net; keydata= xsBLBEXYcVkBCADwSyOk4CDjLYvOENbw7arCVoWRZ7QP5Vwxf9le2jJ8kgdvsAF/vSVB7w69 IFbh58vdMrZPnV2nYEoODQFLS2mNdiwa7o2uo+rE5n6l4ztWM2uffCOCfjGnXSxqVyWkZ+86 Qo5uXIJXkPxRb+W5p7tCtgDZFhW4CL81V383j0XlgcxMS0IvOUb9Ug3yVT1M2tz96E9z8Zs3 ZofzTu8sJQxJcniloItETUy5usOOSgY25raDwx5D2kW1yKw3JFGH19bpK7gwJUkHsLywe6KO 4gtlm+zneyLIFgMxBx42A7G/qxZ6nkf3GQNICuqZn7u0To7TLkXXVPrzNxzwzrUKh9IxAAYp zSNGbGF2aWVuIEdVRURFWiA8ZmxhdmllbkBndWVkZXoubmV0PsLAdAQTAQIAHgIbAwYLCQgH AwIDFQIDAxYCAQIeAQIXgAUCRdhySQAKCRDLlfhEcbLCZTZPB/9AY7O1hpmmWs5L553814u7 eZP+sPIpPi985YEqmRW8p2KRfNRQXMCfkmtxqQCXwFIPC0lD84ZxT3Adqs2TZPjBNwbAxPBn AuiRBK6irS2d3gaN1Xgfy7//CzCJ8sWlwDyMFdnt4BMs1y9l/UpTqhfHwvecRRcGnHiHZRe9 tFRM65aeznFiw0n1PlprMPtKWfo/9wXX7mZEaKsw4F31ik62kAiYZ8Zj+49mNatExCWAV8y6 0sAdbT4qrjMMa0/B6PlL4eRsK85FxpRVNTGzxV7Yfvy4A1rr/H1pJjk0AuQddwswWZW7gcRv gWsQAsfNLUDRQaWBRGjFPn0X9xR8VHMZzsBMBEXYddIBCACjpk6curFwj3kKbzglNv2QarW0 Hmdt/thWr2z3XquYkENWf7ZLMK+0KRJxipB5fehwuR+rj7xnT5upPsSYeq6HyusDLBjvqxsR eYX+9n0pA95CW1oVJKzYpYKMvzvHC/AB7JztmOZ+MuZsQooKD+6cTTQXueUSoPX/4vu6c4Un 87BFb/NjUbEp5arUuOmNraUtJDUEdVW77IVuPkp9+oJcwCYRC7iQxHKa9+gWzEBTc657OMxH 4/EyJaIwlDdskv45zKLCXWPP6EWTiWR9jQaSXy6/TWBOQbKBkouTVDbYdjzbvetCB6cJP1Mq Mc3SXwJMzp+JA8X7kFZCtw0vEor/AAkBAcLAXwQYAQIACQUCRdh10gIbDAAKCRDLlfhEcbLC ZWMTCACavTuBk6oPN4tkTIzCtVyucbSFI8tvQrEBLKLR3Ir42xEYoSTNa4V+hQL9HNxauQnZ JWpWtzqSPIeQMwW/mrxTtJjvse3UzPHGuu47GwSaK9EatE4JuEvmDwcD5zVlo5b4JjG642gy Xnjbwbvvw2+/ER+aEMuoRkWBBWN5l+a/1oRc4N6MKK5sbxLkYnk4Z8fpCbgLGC4xeFiPNqbu Rc8V2tVPnugnHeNdbdrFE3LEuNUtqAqKuutQ6V34LqCX86OmNOyp9OOnE8IdnEM/JXZVg9aS d75u8DYrs1BgLlv86MN6cUWMpti7V1RyXLT2LVtiIePleU+N2dmgWB/R7NSP In-Reply-To: <3116807.1697642067@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------nSk4lodlxKe3x2k9KP8uzdhi Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 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 --------------nSk4lodlxKe3x2k9KP8uzdhi Content-Type: text/x-patch; charset=UTF-8; name="v2-pglz_decompress_check_for_corrupted.patch" Content-Disposition: attachment; filename="v2-pglz_decompress_check_for_corrupted.patch" Content-Transfer-Encoding: base64 LS0tIGEvc3JjL2NvbW1vbi9wZ19semNvbXByZXNzLmMJMjAyMy0xMC0xOCAxMjozNDozMC42 MTYwMDAwMDAgKzAwMDAKKysrIGIvc3JjL2NvbW1vbi9wZ19semNvbXByZXNzLmMJMjAyMy0x MC0xOCAxNjo0MjoxNy44NTYwMDAwMDAgKzAwMDAKQEAgLTczOCw4ICs3MzgsMTAgQEAKIAkJ CQkgKiBzb3VyY2UsIG9yIGlmIHdlIG9idGFpbmVkIG9mZiA9IDAsIHdlIGhhdmUgcHJvYmxl bXMuICAoV2UKIAkJCQkgKiBtdXN0IGNoZWNrIHRoaXMsIGVsc2Ugd2UgcmlzayBhbiBpbmZp bml0ZSBsb29wIGJlbG93IGluIHRoZQogCQkJCSAqIGZhY2Ugb2YgY29ycnVwdCBkYXRhLikK KwkJCQkgKiBBbHNvIGNoZWNrIGlmIHdlIGZlbGwgYmVmb3JlIHRoZSBiZWdpbm5pbmcgb2Yg dGhlCisJCQkJICogZGVzdGluYXRpb24uCiAJCQkJICovCi0JCQkJaWYgKHVubGlrZWx5KHNw ID4gc3JjZW5kIHx8IG9mZiA9PSAwKSkKKwkJCQlpZiAodW5saWtlbHkoc3AgPiBzcmNlbmQg fHwgb2ZmID09IDAgfHwgKGRwIC0gb2ZmKSA8IGRlc3QpKQogCQkJCQlyZXR1cm4gLTE7CiAK IAkJCQkvKgoK --------------nSk4lodlxKe3x2k9KP8uzdhi--