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
Attachments:
pgstat_c_fix.patchtext/x-patch; charset=us-asciiDownload
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
Attachments:
v2-0001-Fix-some-possibly-insecure-usage-of-memcpy-in-pgs.patchtext/x-patch; charset=us-asciiDownload
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