Re: Some doubious code in pgstat.c

At Thu, 5 Nov 2020 11:48:24 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Thu, Nov 5, 2020 at 9:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.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?

-- 
Kyotaro Horiguchi
NTT Open Source Software Center