Horton

Thread

  1. Possible replace of strncpy on xactdesc.c

    Mario Gonzalez <gonzalemario@gmail.com> — 2026-07-03T03:47:58Z

    Hi there,

I've been doing some research about if we could replace strncpy() in
some code. It seems I found one and I'd like your opinion on it to
determine if it's worth the change.

Basically what I found and IMO it's worth the change is on file xactdesc.c

diff --git a/src/backend/access/rmgrdesc/xactdesc.c
b/src/backend/access/rmgrdesc/xactdesc.c
index 4f53d3035cc..0f466a4c27c 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -256,7 +256,7 @@ ParsePrepareRecord(uint8 info, xl_xact_prepare
*xlrec, xl_xact_parsed_prepare *p
        parsed->nabortstats = xlrec->nabortstats;
        parsed->nmsgs = xlrec->ninvalmsgs;

-       strncpy(parsed->twophase_gid, bufptr, xlrec->gidlen);
+       strlcpy(parsed->twophase_gid, bufptr, sizeof(parsed->twophase_gid));
        bufptr += MAXALIGN(xlrec->gidlen);

First of all, I noticed the `memset(parsed, 0, sizeof(*parsed));` that
existed previously, which initially made me think twice about sending
this. Also, thanks to the feedback of a friend, I learnt a previous
refactor of this code was introduced in:
7b8a899bdeb Make pg_waldump report more detail information about
PREPARE TRANSACTION record.

The other functions that are inside the file already use strlcpy() so
maybe the use of current strncpy() on xactdesc.c is just code that
comes from the refactor itself.

I see that `parsed` is a transient decode struct, freshly zeroed each
call by the memset() I talked about above. To me, it sounds safe to do
the swap. But I'm open to ideas.
I'll send a proper patch once some feedback is received but at least
it's compiling and passing local tests.

Thanks!

-- 
Mario Gonzalez
EDB: https://www.enterprisedb.com

  2. Re: Possible replace of strncpy on xactdesc.c

    Daniel Gustafsson <daniel@yesql.se> — 2026-07-06T14:38:37Z

    > On 3 Jul 2026, at 05:47, Mario González Troncoso <gonzalemario@gmail.com> wrote:

> I've been doing some research about if we could replace strncpy() in
> some code. It seems I found one and I'd like your opinion on it to
> determine if it's worth the change.
> 
> Basically what I found and IMO it's worth the change is on file xactdesc.c
> 
> diff --git a/src/backend/access/rmgrdesc/xactdesc.c
> b/src/backend/access/rmgrdesc/xactdesc.c
> index 4f53d3035cc..0f466a4c27c 100644
> --- a/src/backend/access/rmgrdesc/xactdesc.c
> +++ b/src/backend/access/rmgrdesc/xactdesc.c
> @@ -256,7 +256,7 @@ ParsePrepareRecord(uint8 info, xl_xact_prepare
> *xlrec, xl_xact_parsed_prepare *p
>        parsed->nabortstats = xlrec->nabortstats;
>        parsed->nmsgs = xlrec->ninvalmsgs;
> 
> -       strncpy(parsed->twophase_gid, bufptr, xlrec->gidlen);
> +       strlcpy(parsed->twophase_gid, bufptr, sizeof(parsed->twophase_gid));
>        bufptr += MAXALIGN(xlrec->gidlen);

As a general rule it's a good idea to replace strncpy with strlcpy.

> The other functions that are inside the file already use strlcpy() so
> maybe the use of current strncpy() on xactdesc.c is just code that
> comes from the refactor itself.

It was introduced in 1eb6d6527aae in twophase.c and then moved to xaxtdesc.c in
the above mentioned commit.

> I'll send a proper patch once some feedback is received but at least
> it's compiling and passing local tests.

Sounds good, please send a patch.

--
Daniel Gustafsson

  3. Re: Possible replace of strncpy on xactdesc.c

    Mario Gonzalez <gonzalemario@gmail.com> — 2026-07-06T22:26:57Z

    On Mon, 6 Jul 2026 at 10:38, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>
> As a general rule it's a good idea to replace strncpy with strlcpy.
>
> > The other functions that are inside the file already use strlcpy() so
> > maybe the use of current strncpy() on xactdesc.c is just code that
> > comes from the refactor itself.
>
> It was introduced in 1eb6d6527aae in twophase.c and then moved to xaxtdesc.c in
> the above mentioned commit.
>
> > I'll send a proper patch once some feedback is received but at least
> > it's compiling and passing local tests.
>
> Sounds good, please send a patch.
>

Great. Sending it now after rebasing from master and passing local
tests (long live cirrus CI).

I added this to the commitfest as well
https://commitfest.postgresql.org/patch/6989/

>


-- 
Mario Gonzalez
EDB: https://www.enterprisedb.com

  4. Re: Possible replace of strncpy on xactdesc.c

    Mario Gonzalez <gonzalemario@gmail.com> — 2026-07-06T22:31:44Z

    On Mon, 6 Jul 2026 at 18:26, Mario González Troncoso
<gonzalemario@gmail.com> wrote:
>
> On Mon, 6 Jul 2026 at 10:38, Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> >
> > As a general rule it's a good idea to replace strncpy with strlcpy.
> >
> > > The other functions that are inside the file already use strlcpy() so
> > > maybe the use of current strncpy() on xactdesc.c is just code that
> > > comes from the refactor itself.
> >
> > It was introduced in 1eb6d6527aae in twophase.c and then moved to xaxtdesc.c in
> > the above mentioned commit.
> >
> > > I'll send a proper patch once some feedback is received but at least
> > > it's compiling and passing local tests.
> >
> > Sounds good, please send a patch.
> >
>
> Great. Sending it now after rebasing from master and passing local
> tests (long live cirrus CI).
>
> I added this to the commitfest as well
> https://commitfest.postgresql.org/patch/6989/
>
> >
>

Hopefully with the patch.

>
> --
> Mario Gonzalez
> EDB: https://www.enterprisedb.com



-- 
Mario Gonzalez
EDB: https://www.enterprisedb.com