Thread: Some doubious code in pgstat.c
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
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f1dca2f25b..9008601fc4 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -6880,7 +6880,7 @@ pgstat_recv_replslot(PgStat_MsgReplSlot *msg, int len)
if (idx < 0)
return;
- Assert(idx >= 0 && idx <= max_replication_slots);
+ Assert(idx >= 0 && idx < max_replication_slots);
if (msg->m_drop)
{
/* Remove the replication slot statistics with the given name */
@@ -7113,7 +7113,7 @@ pgstat_replslot_index(const char *name, bool create_it)
/* Register new slot */
memset(&replSlotStats[nReplSlotStats], 0, sizeof(PgStat_ReplSlotStats));
- memcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);
+ strlcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);
return nReplSlotStats++;
}
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.
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/
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
From 48d921cf553b67ac2b8ff0c120218e62f4343116 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 5 Nov 2020 10:30:49 +0900
Subject: [PATCH v2] Fix some possibly-insecure usage of memcpy in pgstat.c
Replication slot reporting code has a wrong assertion and some
possibly-insecure use of memcpy. They're not outright a bug but make
them tidy.
---
src/backend/postmaster/pgstat.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f1dca2f25b..b70360c05d 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1489,7 +1489,7 @@ pgstat_reset_replslot_counter(const char *name)
if (SlotIsPhysical(slot))
return;
- memcpy(&msg.m_slotname, name, NAMEDATALEN);
+ strlcpy(msg.m_slotname, name, NAMEDATALEN);
msg.clearall = false;
}
else
@@ -1716,7 +1716,7 @@ pgstat_report_replslot(const char *slotname, int spilltxns, int spillcount,
* Prepare and send the message
*/
pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_REPLSLOT);
- memcpy(&msg.m_slotname, slotname, NAMEDATALEN);
+ strlcpy(msg.m_slotname, slotname, NAMEDATALEN);
msg.m_drop = false;
msg.m_spill_txns = spilltxns;
msg.m_spill_count = spillcount;
@@ -1739,7 +1739,7 @@ pgstat_report_replslot_drop(const char *slotname)
PgStat_MsgReplSlot msg;
pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_REPLSLOT);
- memcpy(&msg.m_slotname, slotname, NAMEDATALEN);
+ strlcpy(msg.m_slotname, slotname, NAMEDATALEN);
msg.m_drop = true;
pgstat_send(&msg, sizeof(PgStat_MsgReplSlot));
}
@@ -6880,7 +6880,7 @@ pgstat_recv_replslot(PgStat_MsgReplSlot *msg, int len)
if (idx < 0)
return;
- Assert(idx >= 0 && idx <= max_replication_slots);
+ Assert(idx >= 0 && idx < max_replication_slots);
if (msg->m_drop)
{
/* Remove the replication slot statistics with the given name */
@@ -7113,7 +7113,7 @@ pgstat_replslot_index(const char *name, bool create_it)
/* Register new slot */
memset(&replSlotStats[nReplSlotStats], 0, sizeof(PgStat_ReplSlotStats));
- memcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);
+ strlcpy(replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);
return nReplSlotStats++;
}
--
2.18.4
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/
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.
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
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.
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.
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