Possible replace of strncpy on xactdesc.c
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
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
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
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
Attachments:
v1-0001-Replace-of-strncpy-on-xactdesc.c.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Replace-of-strncpy-on-xactdesc.c.patchDownload+1-2
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
Hello,
At Mon, 6 Jul 2026 18:31:44 -0400, Mario González Troncoso <gonzalemario@gmail.com> wrote in
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/
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
On 9 Jul 2026, at 08:15, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
Wouldn't memcpy(parsed->twophase_gid, bufptr, xlrec->gidlen) express the
intent more directly?
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