public inbox for [email protected]help / color / mirror / Atom feed
Possible replace of strncpy on xactdesc.c 7+ messages / 4 participants [nested] [flat]
* Possible replace of strncpy on xactdesc.c @ 2026-07-03 03:47 Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Mario González Troncoso @ 2026-07-03 03:47 UTC (permalink / raw) To: pgsql-hackers 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> @ 2026-07-06 14:38 ` Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Daniel Gustafsson @ 2026-07-06 14:38 UTC (permalink / raw) To: Mario González Troncoso <[email protected]>; +Cc: pgsql-hackers > On 3 Jul 2026, at 05:47, Mario González Troncoso <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> @ 2026-07-06 22:26 ` Mario González Troncoso <[email protected]> 2026-07-06 22:31 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Mario González Troncoso @ 2026-07-06 22:26 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers On Mon, 6 Jul 2026 at 10:38, Daniel Gustafsson <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> @ 2026-07-06 22:31 ` Mario González Troncoso <[email protected]> 2026-07-09 05:03 ` Re: Possible replace of strncpy on xactdesc.c solai v <[email protected]> 2026-07-09 06:15 ` Re: Possible replace of strncpy on xactdesc.c Kyotaro Horiguchi <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Mario González Troncoso @ 2026-07-06 22:31 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers On Mon, 6 Jul 2026 at 18:26, Mario González Troncoso <[email protected]> wrote: > > On Mon, 6 Jul 2026 at 10:38, Daniel Gustafsson <[email protected]> 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: [text/x-patch] v1-0001-Replace-of-strncpy-on-xactdesc.c.patch (1.5K, ../../CAFsReFVg--+gp8T_QzKos6aUPZRfOupBz=d=1sHg2wc8KkO_WQ@mail.gmail.com/2-v1-0001-Replace-of-strncpy-on-xactdesc.c.patch) download | inline diff: From 3e1e277f34e3d8a2bfd7df06be4e56683fab6199 Mon Sep 17 00:00:00 2001 From: Mario Gonzalez <[email protected]> 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; - strncpy(parsed->twophase_gid, bufptr, xlrec->gidlen); + strlcpy(parsed->twophase_gid, bufptr, sizeof(parsed->twophase_gid)); bufptr += MAXALIGN(xlrec->gidlen); parsed->subxacts = (TransactionId *) bufptr; -- 2.47.3 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 22:31 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> @ 2026-07-09 05:03 ` solai v <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: solai v @ 2026-07-09 05:03 UTC (permalink / raw) To: Mario González Troncoso <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 22:31 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> @ 2026-07-09 06:15 ` Kyotaro Horiguchi <[email protected]> 2026-07-09 08:00 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Kyotaro Horiguchi @ 2026-07-09 06:15 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; pgsql-hackers Hello, At Mon, 6 Jul 2026 18:31:44 -0400, Mario González Troncoso <[email protected]> wrote in > On Mon, 6 Jul 2026 at 18:26, Mario González Troncoso > <[email protected]> wrote: > > > > On Mon, 6 Jul 2026 at 10:38, Daniel Gustafsson <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Possible replace of strncpy on xactdesc.c 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Re: Possible replace of strncpy on xactdesc.c Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 22:31 ` Re: Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-09 06:15 ` Re: Possible replace of strncpy on xactdesc.c Kyotaro Horiguchi <[email protected]> @ 2026-07-09 08:00 ` Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Daniel Gustafsson @ 2026-07-09 08:00 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Mario González Troncoso <[email protected]>; pgsql-hackers > On 9 Jul 2026, at 08:15, Kyotaro Horiguchi <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-07-09 08:00 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-03 03:47 Possible replace of strncpy on xactdesc.c Mario González Troncoso <[email protected]> 2026-07-06 14:38 ` Daniel Gustafsson <[email protected]> 2026-07-06 22:26 ` Mario González Troncoso <[email protected]> 2026-07-06 22:31 ` Mario González Troncoso <[email protected]> 2026-07-09 05:03 ` solai v <[email protected]> 2026-07-09 06:15 ` Kyotaro Horiguchi <[email protected]> 2026-07-09 08:00 ` Daniel Gustafsson <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox