Re: Some doubious code in pgstat.c

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?

--
Kyotaro Horiguchi
NTT Open Source Software Center