Horton

Thread

Commits

  1. Fix assert in logicalmsg_desc

  1. bogus assert in logicalmsg_desc

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2022-08-14T16:16:53Z

    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

  2. Re: bogus assert in logicalmsg_desc

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-08-15T01:13:35Z

    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/

  3. Re: bogus assert in logicalmsg_desc

    Richard Guo <guofenglinux@gmail.com> — 2022-08-15T01:20:28Z

    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