bogus assert in logicalmsg_desc
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.patchtext/x-patch; charset=UTF-8; name=logicalmsgdesc-fix.patchDownload
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",
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/
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