Re: Some doubious code in pgstat.c

On Thu, Nov 5, 2020 at 2:13 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Thu, 5 Nov 2020 11:48:24 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in
> > On Thu, Nov 5, 2020 at 9:44 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi
> > > <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > > > As another issue, just replace memcpy with strlcpy makes compiler
> > > > complain of type mismatch, as the first paramter to memcpy had an
> > > > needless "&" operator. I removed it in this patch.
> > > >
> > > > (&msg.m_slotname is a "char (*)[NAMEDATALEN]", not a "char *".)
> > > >
> > >
> > > The patch looks good to me.
> > >
> >
> > LGTM as well but the proposed commit message seems to be a bit
> > unclear. How about something like this:
> > "Use strlcpy instead of memcpy for copying the slot name in pgstat.c.
> >
> > There is no outright bug here but it is better to be consistent with
> > the usage at other places in the same file. In the passing, fix a wrong
> > Assertion in pgstat_recv_replslot."
>
> Looks better, thanks.
>
> By the way, I noticed the following sequence.
>
> pgstat.c: 3204
> 3204> lbeentry.st_appname[0] = '\0';
> 3205> if (MyProcPort && MyProcPort->remote_hostname)
> 3206> strlcpy(lbeentry.st_clienthostname, MyProcPort->remote_hostname,
> 3207> NAMEDATALEN);
> 3208> else
> 3209> lbeentry.st_clienthostname[0] = '\0';
> 3210> lbeentry.st_activity_raw[0] = '\0';
> 3211> /* Also make sure the last byte in each string area is always 0 */
> 3212> lbeentry.st_appname[NAMEDATALEN - 1] = '\0';
> 3213> lbeentry.st_clienthostname[NAMEDATALEN - 1] = '\0';
> 3214> lbeentry.st_activity_raw[pgstat_track_activity_query_size - 1] = '\0';
>
>
> The strlcpy at the line 3206 makes sure that st_clienthostname is
> null-terminated so it's nonsense to do line 3213. st_appname and
> st_activity_raw are set to zero-length string.
>
> Is there any point in setting terminating nul to them?
>

I also don't see any reason for the same except being extra careful.
This is not directly related to this patch so I think we can leave
this or if you want you can discuss this in a separate thread. It
seems to be introduced in commit 85ccb689.

--
With Regards,
Amit Kapila.