pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedbogus assert in logicalmsg_desc
3+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-08-16 c52ad9c4ef be6aadf07d 239c3ee41b Fix assert in logicalmsg_desc
* bogus assert in logicalmsg_desc
@ 2022-08-14 16:16 Tomas Vondra <tomas.vondra@enterprisedb.com>
0 siblings, 2 replies; 3+ messages in thread
From: Tomas Vondra @ 2022-08-14 16:16 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
while experimenting with logical messages, I ran into this assert in
logicalmsg_desc:
Assert(prefix[xlrec->prefix_size] != '\0');
This seems to be incorrect, because LogLogicalMessage does this:
xlrec.prefix_size = strlen(prefix) + 1;
So prefix_size includes the null byte, so the assert points out at the
first payload byte. And of course, the check should be "==" because we
expect the byte to be \0, not the other way around.
It's pretty simple to make this crash by writing a logical message where
the first payload byte is \0, e.g. like this:
select pg_logical_emit_message(true, 'a'::text, '\x00'::bytea);
and then running pg_waldump on the WAL segment.
Attached is a patch addressing this. This was added in 14, so we should
backpatch to that version.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
logicalmsgdesc-fix.patch (text/x-patch, 642B) download
diff --git a/src/backend/access/rmgrdesc/logicalmsgdesc.c b/src/backend/access/rmgrdesc/logicalmsgdesc.c
index 099e11a84e7..08e03aa30d1 100644
--- a/src/backend/access/rmgrdesc/logicalmsgdesc.c
+++ b/src/backend/access/rmgrdesc/logicalmsgdesc.c
@@ -28,7 +28,7 @@ logicalmsg_desc(StringInfo buf, XLogReaderState *record)
char *message = xlrec->message + xlrec->prefix_size;
char *sep = "";
- Assert(prefix[xlrec->prefix_size] != '\0');
+ Assert(prefix[xlrec->prefix_size - 1] == '\0');
appendStringInfo(buf, "%s, prefix \"%s\"; payload (%zu bytes): ",
xlrec->transactional ? "transactional" : "non-transactional",
* Re: bogus assert in logicalmsg_desc
@ 2022-08-15 01:13 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Tomas Vondra <tomas.vondra@enterprisedb.com>
1 sibling, 0 replies; 3+ messages in thread
From: Masahiko Sawada @ 2022-08-15 01:13 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@enterprisedb.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Aug 15, 2022 at 1:17 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> Hi,
>
> while experimenting with logical messages, I ran into this assert in
> logicalmsg_desc:
>
> Assert(prefix[xlrec->prefix_size] != '\0');
>
> This seems to be incorrect, because LogLogicalMessage does this:
>
> xlrec.prefix_size = strlen(prefix) + 1;
>
> So prefix_size includes the null byte, so the assert points out at the
> first payload byte. And of course, the check should be "==" because we
> expect the byte to be \0, not the other way around.
>
> It's pretty simple to make this crash by writing a logical message where
> the first payload byte is \0, e.g. like this:
>
> select pg_logical_emit_message(true, 'a'::text, '\x00'::bytea);
>
> and then running pg_waldump on the WAL segment.
>
> Attached is a patch addressing this. This was added in 14, so we should
> backpatch to that version.
+1
The patch looks good to me.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: bogus assert in logicalmsg_desc
@ 2022-08-15 01:20 Richard Guo <guofenglinux@gmail.com>
parent: Tomas Vondra <tomas.vondra@enterprisedb.com>
1 sibling, 0 replies; 3+ messages in thread
From: Richard Guo @ 2022-08-15 01:20 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@enterprisedb.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Aug 15, 2022 at 12:17 AM Tomas Vondra <tomas.vondra@enterprisedb.com>
wrote:
> So prefix_size includes the null byte, so the assert points out at the
> first payload byte. And of course, the check should be "==" because we
> expect the byte to be \0, not the other way around.
Yes, indeed. There is even a comment emphasizing the trailing null byte
in LogLogicalMessage.
/* trailing zero is critical; see logicalmsg_desc */
>
> Attached is a patch addressing this. This was added in 14, so we should
> backpatch to that version.
>
+1 for the patch.
Thanks
Richard
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-08-15 01:20 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-08-14 16:16 bogus assert in logicalmsg_desc Tomas Vondra <tomas.vondra@enterprisedb.com>
2022-08-15 01:13 ` Masahiko Sawada <sawada.mshk@gmail.com>
2022-08-15 01:20 ` Richard Guo <guofenglinux@gmail.com>
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