Horton

Thread

Commits

  1. Use strlcpy instead of memcpy for copying the slot name in pgstat.c.

  2. Rearrange pgstat_bestart() to avoid failures within its critical section.

  1. Some doubious code in pgstat.c

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-04T08:55:23Z

    Hello.

While updating a patch, I noticed that the replication slot stats
patch (9868167500) put some somewhat doubious codes.

In pgstat_recv_replslot, an assertion like the following exists:

>	idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop);
..
>	Assert(idx >= 0 && idx < max_replication_slots);

But the idx should be 0..(max_replication_slots - 1).


In the same function the following code assumes that the given "char
*name" has the length of NAMEDATALEN.  It actually is, but that
assumption seems a bit bogus. I think it should use strlcpy instead.


>pgstat_replslot_index(const char *name, bool create_it)
...
>	memcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: Some doubious code in pgstat.c

    Amit Kapila <amit.kapila16@gmail.com> — 2020-11-04T09:50:04Z

    On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Hello.
>
> While updating a patch, I noticed that the replication slot stats
> patch (9868167500) put some somewhat doubious codes.
>
> In pgstat_recv_replslot, an assertion like the following exists:
>
> >       idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop);
> ..
> >       Assert(idx >= 0 && idx < max_replication_slots);
>
> But the idx should be 0..(max_replication_slots - 1).
>

Right.

>
> In the same function the following code assumes that the given "char
> *name" has the length of NAMEDATALEN.  It actually is, but that
> assumption seems a bit bogus. I think it should use strlcpy instead.
>

Agreed.

Your patch looks good to me.

-- 
With Regards,
Amit Kapila.

  3. Re: Some doubious code in pgstat.c

    Masahiko Sawada <sawada.mshk@gmail.com> — 2020-11-04T13:49:57Z

    On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Hello.
> >
> > While updating a patch, I noticed that the replication slot stats
> > patch (9868167500) put some somewhat doubious codes.
> >
> > In pgstat_recv_replslot, an assertion like the following exists:
> >
> > >       idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop);
> > ..
> > >       Assert(idx >= 0 && idx < max_replication_slots);
> >
> > But the idx should be 0..(max_replication_slots - 1).
> >
>
> Right.
>
> >
> > In the same function the following code assumes that the given "char
> > *name" has the length of NAMEDATALEN.  It actually is, but that
> > assumption seems a bit bogus. I think it should use strlcpy instead.
> >
>
> Agreed.

+1

The commit uses memcpy in the same way in other places too, for
instance in pgstat_report_replslot_drop(). Should we fix all of them?

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

  4. Re: Some doubious code in pgstat.c

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-05T02:18:42Z

    At Wed, 4 Nov 2020 22:49:57 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > Hello.
> > >
> > > While updating a patch, I noticed that the replication slot stats
> > > patch (9868167500) put some somewhat doubious codes.
> > >
> > > In pgstat_recv_replslot, an assertion like the following exists:
> > >
> > > >       idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop);
> > > ..
> > > >       Assert(idx >= 0 && idx < max_replication_slots);
> > >
> > > But the idx should be 0..(max_replication_slots - 1).
> > >
> >
> > Right.
> >
> > >
> > > In the same function the following code assumes that the given "char
> > > *name" has the length of NAMEDATALEN.  It actually is, but that
> > > assumption seems a bit bogus. I think it should use strlcpy instead.
> > >
> >
> > Agreed.
> 
> +1
> 
> The commit uses memcpy in the same way in other places too, for
> instance in pgstat_report_replslot_drop(). Should we fix all of them?

Absolutely. the same is seen at several places.  Please find the
attached.

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 *".)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: Some doubious code in pgstat.c

    Masahiko Sawada <sawada.mshk@gmail.com> — 2020-11-05T04:14:20Z

    On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 4 Nov 2020 22:49:57 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
> > > <horikyota.ntt@gmail.com> wrote:
> > > >
> > > > Hello.
> > > >
> > > > While updating a patch, I noticed that the replication slot stats
> > > > patch (9868167500) put some somewhat doubious codes.
> > > >
> > > > In pgstat_recv_replslot, an assertion like the following exists:
> > > >
> > > > >       idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop);
> > > > ..
> > > > >       Assert(idx >= 0 && idx < max_replication_slots);
> > > >
> > > > But the idx should be 0..(max_replication_slots - 1).
> > > >
> > >
> > > Right.
> > >
> > > >
> > > > In the same function the following code assumes that the given "char
> > > > *name" has the length of NAMEDATALEN.  It actually is, but that
> > > > assumption seems a bit bogus. I think it should use strlcpy instead.
> > > >
> > >
> > > Agreed.
> >
> > +1
> >
> > The commit uses memcpy in the same way in other places too, for
> > instance in pgstat_report_replslot_drop(). Should we fix all of them?
>
> Absolutely. the same is seen at several places.  Please find the
> attached.
>
> 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.

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

  6. Re: Some doubious code in pgstat.c

    Amit Kapila <amit.kapila16@gmail.com> — 2020-11-05T06:18:24Z

    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."

-- 
With Regards,
Amit Kapila.

  7. Re: Some doubious code in pgstat.c

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-05T08:43:34Z

    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

  8. Re: Some doubious code in pgstat.c

    Amit Kapila <amit.kapila16@gmail.com> — 2020-11-05T09:20:38Z

    On Thu, Nov 5, 2020 at 2:13 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> 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?
>

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.

  9. Re: Some doubious code in pgstat.c

    Amit Kapila <amit.kapila16@gmail.com> — 2020-11-06T11:10:39Z

    On Thu, Nov 5, 2020 at 2:13 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> 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.
>

Pushed!

-- 
With Regards,
Amit Kapila.

  10. Re: Some doubious code in pgstat.c

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-09T02:48:51Z

    At Fri, 6 Nov 2020 16:40:39 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Thu, Nov 5, 2020 at 2:13 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > 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.
> >
> 
> Pushed!

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center