Re: Some doubious code in pgstat.c - Mailing list pgsql-hackers

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