pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_dump: Fix dangling pointer in EndCompressorZstd()
6+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2025-04-17 b669293e34 pg_dump: Set private_date pointer to NULL in callback
* pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-16 08:27 Alexander Kuznetsov <kuznetsovam@altlinux.org>
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Kuznetsov @ 2025-04-16 08:27 UTC (permalink / raw)
To: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; +Cc: egori@altlinux.org; nickel@altlinux.org
Hello everyone,
We've found that EndCompressorZstd() doesn't set cs->private_data to NULL after pg_free(),
unlike other EndCompressor implementations.
While this doesn't currently cause issues (as the pointer soon gets reassigned),
we recommend fixing this to maintain consistency with other implementations and prevent potential future issues.
The patch is attached, would appreciate your thoughts on this change.
--
Best regards,
Alexander Kuznetsov
Attachments:
0001-pg_dump-Fix-dangling-pointer-in-EndCompressorZstd.patch (text/x-patch, 1.0K) download
From 428c60888f96aa5d0b7575a4342cdce4ff0257ab Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Date: Wed, 16 Apr 2025 11:19:56 +0300
Subject: [PATCH] pg_dump: Fix dangling pointer in EndCompressorZstd()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
cs->private_data becomes dangling after pg_free() call and should be set to NULL
(consistent with other EndCompressor implementations)
Found by Linux Verification Center (linuxtesting.org) with Svace.
---
src/bin/pg_dump/compress_zstd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/pg_dump/compress_zstd.c b/src/bin/pg_dump/compress_zstd.c
index 1f7b4942706..cb595b10c2d 100644
--- a/src/bin/pg_dump/compress_zstd.c
+++ b/src/bin/pg_dump/compress_zstd.c
@@ -142,6 +142,7 @@ EndCompressorZstd(ArchiveHandle *AH, CompressorState *cs)
/* output buffer may be allocated in either mode */
pg_free(zstdcs->output.dst);
pg_free(zstdcs);
+ cs->private_data = NULL;
}
static void
--
2.42.4
* Re: pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-16 11:48 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Alexander Kuznetsov <kuznetsovam@altlinux.org>
0 siblings, 1 reply; 6+ messages in thread
From: Ashutosh Bapat @ 2025-04-16 11:48 UTC (permalink / raw)
To: kuznetsovam@altlinux.org; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; egori@altlinux.org; nickel@altlinux.org
On Wed, Apr 16, 2025 at 1:57 PM Alexander Kuznetsov
<kuznetsovam@altlinux.org> wrote:
>
> Hello everyone,
>
> We've found that EndCompressorZstd() doesn't set cs->private_data to NULL after pg_free(),
> unlike other EndCompressor implementations.
> While this doesn't currently cause issues (as the pointer soon gets reassigned),
> we recommend fixing this to maintain consistency with other implementations and prevent potential future issues.
>
> The patch is attached, would appreciate your thoughts on this change.
Thanks for the patch.
The next thing that happens in EndCompressor() is freeing cs itself.
So cs->private_data is not used anywhere, so no harm in the current
code. But it's better to set to NULL since EndCompressorZstd()
wouldn't know how it's being accessed after returning from there. The
other implementation of CompressionState::end() EndCompressorGzip()
calls DeflateCompressorEnd() which also sets cs->private_data
explicitly. So should EndCompressorZstd().
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-16 14:19 Daniel Gustafsson <daniel@yesql.se>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Gustafsson @ 2025-04-16 14:19 UTC (permalink / raw)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; +Cc: kuznetsovam@altlinux.org, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; egori@altlinux.org; nickel@altlinux.org
> On 16 Apr 2025, at 13:48, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Apr 16, 2025 at 1:57 PM Alexander Kuznetsov
> <kuznetsovam@altlinux.org> wrote:
>>
>> Hello everyone,
>>
>> We've found that EndCompressorZstd() doesn't set cs->private_data to NULL after pg_free(),
>> unlike other EndCompressor implementations.
>> While this doesn't currently cause issues (as the pointer soon gets reassigned),
>> we recommend fixing this to maintain consistency with other implementations and prevent potential future issues.
>>
>> The patch is attached, would appreciate your thoughts on this change.
>
> Thanks for the patch.
>
> The next thing that happens in EndCompressor() is freeing cs itself.
> So cs->private_data is not used anywhere, so no harm in the current
> code. But it's better to set to NULL since EndCompressorZstd()
> wouldn't know how it's being accessed after returning from there. The
> other implementation of CompressionState::end() EndCompressorGzip()
> calls DeflateCompressorEnd() which also sets cs->private_data
> explicitly. So should EndCompressorZstd().
Agreed, while it's perfectly safe today the end method should not make
assumptions about the use of the private_data pointer upon return and should
leave it set to NULL.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-16 22:29 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2025-04-16 22:29 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; kuznetsovam@altlinux.org, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; egori@altlinux.org; nickel@altlinux.org
On Wed, Apr 16, 2025 at 04:19:02PM +0200, Daniel Gustafsson wrote:
> Agreed, while it's perfectly safe today the end method should not make
> assumptions about the use of the private_data pointer upon return and should
> leave it set to NULL.
Indeed. I was just looking at applying what Alexander has sent
because what EndCompressorZstd() not doing what the other methods do
makes no sense. Perhaps you are already on it, Daniel?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-16 23:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2025-04-16 23:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; kuznetsovam@altlinux.org, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; egori@altlinux.org; nickel@altlinux.org
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Apr 16, 2025 at 04:19:02PM +0200, Daniel Gustafsson wrote:
>> Agreed, while it's perfectly safe today the end method should not make
>> assumptions about the use of the private_data pointer upon return and should
>> leave it set to NULL.
> Indeed. I was just looking at applying what Alexander has sent
> because what EndCompressorZstd() not doing what the other methods do
> makes no sense. Perhaps you are already on it, Daniel?
I think the actual reason for the difference is that the methods that
are taking care to zero the pointer do so because they test the
pointer themselves. For instance in EndCompressorGzip, the test is
needed because perhaps no data was sent so the struct never got made.
It incidentally offers protection against a double call of that
function, but I don't think that was the intended reason.
I don't have any big objection to zeroing the pointer in
EndCompressorZstd, but I think the claim that it's precisely
analogous to the other EndCompressor methods is faulty,
because it has no similar test.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pg_dump: Fix dangling pointer in EndCompressorZstd()
@ 2025-04-17 11:01 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Gustafsson @ 2025-04-17 11:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; kuznetsovam@altlinux.org, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>; egori@altlinux.org; nickel@altlinux.org
> On 17 Apr 2025, at 01:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
>> On Wed, Apr 16, 2025 at 04:19:02PM +0200, Daniel Gustafsson wrote:
>>> Agreed, while it's perfectly safe today the end method should not make
>>> assumptions about the use of the private_data pointer upon return and should
>>> leave it set to NULL.
>
>> Indeed. I was just looking at applying what Alexander has sent
>> because what EndCompressorZstd() not doing what the other methods do
>> makes no sense. Perhaps you are already on it, Daniel?
>
> I think the actual reason for the difference is that the methods that
> are taking care to zero the pointer do so because they test the
> pointer themselves. For instance in EndCompressorGzip, the test is
> needed because perhaps no data was sent so the struct never got made.
> It incidentally offers protection against a double call of that
> function, but I don't think that was the intended reason.
>
> I don't have any big objection to zeroing the pointer in
> EndCompressorZstd, but I think the claim that it's precisely
> analogous to the other EndCompressor methods is faulty,
> because it has no similar test.
Right, it has no similar test as the state in private_data is needed for both
read and write whereas gzip for example only need it for write (deflate).
Pushed as it improves code hygiene.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-04-17 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-16 08:27 pg_dump: Fix dangling pointer in EndCompressorZstd() Alexander Kuznetsov <kuznetsovam@altlinux.org>
2025-04-16 11:48 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-04-16 14:19 ` Daniel Gustafsson <daniel@yesql.se>
2025-04-16 22:29 ` Michael Paquier <michael@paquier.xyz>
2025-04-16 23:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-04-17 11:01 ` Daniel Gustafsson <daniel@yesql.se>
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