Subject: Possible replace of strncpy on xactdesc.c

From: Mario González Troncoso <[email-masked]>
Date: Thu, 2 Jul 2026 23:47:58 -0400

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

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 6 Jul 2026 16:38:37 +0200

> On 3 Jul 2026, at 05:47, Mario González Troncoso <[email-masked]> wrote:

>        bufptr += MAXALIGN(xlrec->gidlen);

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

> comes from the refactor itself.

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

> it's compiling and passing local tests.

Sounds good, please send a patch.

--
Daniel Gustafsson

---

From: Mario González Troncoso <[email-masked]>
Date: Mon, 6 Jul 2026 18:26:57 -0400

> 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/

---

From: Mario González Troncoso <[email-masked]>
Date: Mon, 6 Jul 2026 18:31:44 -0400

> https://commitfest.postgresql.org/patch/6989/

Hopefully with the patch.

-- 
Mario Gonzalez
EDB: https://www.enterprisedb.com
From 3e1e277f34e3d8a2bfd7df06be4e56683fab6199 Mon Sep 17 00:00:00 2001
From: Mario Gonzalez <[email-masked]>
Date: Thu, 2 Jul 2026 23:30:34 -0400
Subject: [PATCH v1] Replace of strncpy on xactdesc.c

ParsePrepareRecord function parses data from a xl_xact_prepare struct.
During its execution, it uses strncpy() to copy some data over. This
call can be replaced by strlcpy() from src/port/.

Despite the space in memory used as a destination is zero'ed beforehand
using the memset below, the rest of functions are already using strlcpy:
memset(parsed, 0, sizeof(*parsed));

It seems like accidentally 'n' was typed when 'l' was supposed to be used.
The change with strncpy was introduced in 1eb6d6527aae in twophase.c and
then moved to xaxtdesc.c in 7b8a899bdeb.
---
 src/backend/access/rmgrdesc/xactdesc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;

 	parsed->subxacts = (TransactionId *) bufptr;
-- 
2.47.3

---

From: solai v <[email-masked]>
Date: Thu, 9 Jul 2026 10:33:18 +0530

Hi Mario,

I tested the patch locally.
Before applying the patch, I verified that strncpy() was used in
ParsePrepareRecord() and tested the affected code path by creating and
committing a prepared transaction using PREPARE TRANSACTION. The
prepared transaction appeared correctly in pg_prepared_xacts, and
COMMIT PREPARED completed successfully.

After applying the patch, I rebuilt PostgreSQL and repeated the same
test. The behavior remained unchanged: PREPARE TRANSACTION, COMMIT
PREPARED, and the committed data all worked as expected.
I didn't notice any regressions during my testing. The change looks good to me.

Thanks for the patch!

Regards
Solai

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 09 Jul 2026 15:15:40 +0900 (JST)

Hello,

At Mon, 6 Jul 2026 18:31:44 -0400, Mario González Troncoso <[email-masked]> wrote in 
> > https://commitfest.postgresql.org/patch/6989/

I agree that replacing strncpy() with strlcpy() is often a good general
direction, but I'm not sure this case fits that pattern.

Here, xlrec->gidlen is the length of the GID including the terminating
NUL byte, and the following pointer advance is based on the same length.
So this looks more like copying a known-length field from the WAL record
than copying an arbitrary C string.

Wouldn't memcpy(parsed->twophase_gid, bufptr, xlrec->gidlen) express the
intent more directly?

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 9 Jul 2026 10:00:54 +0200

> On 9 Jul 2026, at 08:15, Kyotaro Horiguchi <[email-masked]> wrote:

> Wouldn't memcpy(parsed->twophase_gid, bufptr, xlrec->gidlen) express the

memcpy would relax the check for twophase_gid being a char array, which albeit
being a small protection still seems like an API characteristic worth keeping.

--
Daniel Gustafsson