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
