Fix out-of-bounds in the function GetCommandTagName
Hi,
Per Coverity.
Coverity has reported some out-of-bounds bugs
related to the GetCommandTagName function.
CID 1542964: (#1 of 1): Out-of-bounds access (OVERRUN)
7. overrun-call: Overrunning callee's array of size 193 by passing argument
commandtag (which evaluates to 193) in call to GetCommandTagName.[
It turns out that the root of the problem is found in the declaration of
the tag_behavior array, which is found in src/backend/tcop/cmdtag.c.
The size of the array is defined by COMMAND_TAG_NEXTTAG enum,
whose value currently corresponds to 193.
Since enum items are evaluated starting at zero, by default.
It turns out that the final size of the array, 193, limits the number of
items to 192, which excludes the last TAG
PG_CMDTAG(CMDTAG_VACUUM, "VACUUM", false, false, false)
Fixed leaving it up to the compiler to determine the final size of the
array.
Patch attached.
best regards,
Ranier Vilela
On Mon, 15 Apr 2024 at 11:17, Ranier Vilela <ranier.vf@gmail.com> wrote:
Coverity has reported some out-of-bounds bugs
related to the GetCommandTagName function.The size of the array is defined by COMMAND_TAG_NEXTTAG enum,
whose value currently corresponds to 193.
Since enum items are evaluated starting at zero, by default.
I think the change makes sense. I don't see any good reason to define
COMMAND_TAG_NEXTTAG or force the compiler's hand when it comes to
sizing that array.
Clearly, Coverity does not understand that we'll never call any of
those GetCommandTag* functions with COMMAND_TAG_NEXTTAG.
Patch attached.
You seem to have forgotten to attach it, but my comments above were
written with the assumption that the patch is what I've attached here.
David
Attachments:
get_rid_of_command_tag_nexttag.patchtext/plain; charset=US-ASCII; name=get_rid_of_command_tag_nexttag.patchDownload
diff --git a/src/backend/tcop/cmdtag.c b/src/backend/tcop/cmdtag.c
index 68689b3e0d..0870064fdd 100644
--- a/src/backend/tcop/cmdtag.c
+++ b/src/backend/tcop/cmdtag.c
@@ -30,7 +30,7 @@ typedef struct CommandTagBehavior
#define PG_CMDTAG(tag, name, evtrgok, rwrok, rowcnt) \
{ name, (uint8) (sizeof(name) - 1), evtrgok, rwrok, rowcnt },
-static const CommandTagBehavior tag_behavior[COMMAND_TAG_NEXTTAG] = {
+static const CommandTagBehavior tag_behavior[] = {
#include "tcop/cmdtaglist.h"
};
diff --git a/src/include/tcop/cmdtag.h b/src/include/tcop/cmdtag.h
index da210e54fa..23c99d7eca 100644
--- a/src/include/tcop/cmdtag.h
+++ b/src/include/tcop/cmdtag.h
@@ -22,7 +22,6 @@
typedef enum CommandTag
{
#include "tcop/cmdtaglist.h"
- COMMAND_TAG_NEXTTAG
} CommandTag;
#undef PG_CMDTAG
David Rowley <dgrowleyml@gmail.com> writes:
I think the change makes sense. I don't see any good reason to define
COMMAND_TAG_NEXTTAG or force the compiler's hand when it comes to
sizing that array.
Clearly, Coverity does not understand that we'll never call any of
those GetCommandTag* functions with COMMAND_TAG_NEXTTAG.
+1, but would this also allow us to get rid of any default:
cases in switches on command tags?
regards, tom lane
Em dom., 14 de abr. de 2024 às 20:38, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Mon, 15 Apr 2024 at 11:17, Ranier Vilela <ranier.vf@gmail.com> wrote:
Coverity has reported some out-of-bounds bugs
related to the GetCommandTagName function.The size of the array is defined by COMMAND_TAG_NEXTTAG enum,
whose value currently corresponds to 193.
Since enum items are evaluated starting at zero, by default.I think the change makes sense. I don't see any good reason to define
COMMAND_TAG_NEXTTAG or force the compiler's hand when it comes to
sizing that array.Clearly, Coverity does not understand that we'll never call any of
those GetCommandTag* functions with COMMAND_TAG_NEXTTAG.
I think that Coverity understood it this way because when
including COMMAND_TAG_NEXTTAG, in the enum definition,
led to 193 items, and the last item in the array is currently 192.
Patch attached.
You seem to have forgotten to attach it, but my comments above were
written with the assumption that the patch is what I've attached here.
Yes, I actually forgot.
+1 for your patch.
best regards,
Ranier Vilela
On Mon, 15 Apr 2024 at 11:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
would this also allow us to get rid of any default:
cases in switches on command tags?
git grep "case CMDTAG_" does not yield any results.
As far as I understand, we'd only be able to get rid of a default case
if we had a switch that included all CMDTAG* values apart from
COMMAND_TAG_NEXTTAG. If we don't ever switch on CMDTAG values then I
think the answer to your question is "no".
David
David Rowley <dgrowleyml@gmail.com> writes:
On Mon, 15 Apr 2024 at 11:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
would this also allow us to get rid of any default:
cases in switches on command tags?
git grep "case CMDTAG_" does not yield any results.
OK. It was worth checking.
regards, tom lane
On Mon, 15 Apr 2024 at 12:12, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em dom., 14 de abr. de 2024 às 20:38, David Rowley <dgrowleyml@gmail.com> escreveu:
You seem to have forgotten to attach it, but my comments above were
written with the assumption that the patch is what I've attached here.Yes, I actually forgot.
+1 for your patch.
I've added a CF entry under your name for this:
https://commitfest.postgresql.org/48/4927/
If it was code new to PG17 I'd be inclined to go ahead with it now,
but it does not seem to align with making the release mode stable.t
I'd bet others will feel differently about that. Delaying seems a
better default choice at least.
David
Em dom., 14 de abr. de 2024 às 23:12, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Mon, 15 Apr 2024 at 12:12, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em dom., 14 de abr. de 2024 às 20:38, David Rowley <dgrowleyml@gmail.com>
escreveu:
You seem to have forgotten to attach it, but my comments above were
written with the assumption that the patch is what I've attached here.Yes, I actually forgot.
+1 for your patch.
I've added a CF entry under your name for this:
https://commitfest.postgresql.org/48/4927/
Thank you.
If it was code new to PG17 I'd be inclined to go ahead with it now,
but it does not seem to align with making the release mode stable.t
I'd bet others will feel differently about that. Delaying seems a
better default choice at least.
I agree. Although I initially thought it was a live bug, that's actually
not the case.
In fact, this is a refactoring.
best regards,
Ranier Vilela
David Rowley <dgrowleyml@gmail.com> writes:
I've added a CF entry under your name for this:
https://commitfest.postgresql.org/48/4927/
If it was code new to PG17 I'd be inclined to go ahead with it now,
but it does not seem to align with making the release mode stable.
I'd bet others will feel differently about that. Delaying seems a
better default choice at least.
The security team's Coverity instance has started to show this
complaint now too. So I'm going to go ahead and push this change
in HEAD. It's probably unwise to change it in stable branches,
since there's at least a small chance some external code is using
COMMAND_TAG_NEXTTAG for the same purpose tag_behavior[] does.
But we aren't anywhere near declaring v17's API stable, so
I'd rather fix the issue than dismiss it in HEAD.
regards, tom lane
Em seg., 13 de mai. de 2024 às 14:38, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
David Rowley <dgrowleyml@gmail.com> writes:
I've added a CF entry under your name for this:
https://commitfest.postgresql.org/48/4927/If it was code new to PG17 I'd be inclined to go ahead with it now,
but it does not seem to align with making the release mode stable.
I'd bet others will feel differently about that. Delaying seems a
better default choice at least.The security team's Coverity instance has started to show this
complaint now too. So I'm going to go ahead and push this change
in HEAD. It's probably unwise to change it in stable branches,
since there's at least a small chance some external code is using
COMMAND_TAG_NEXTTAG for the same purpose tag_behavior[] does.
But we aren't anywhere near declaring v17's API stable, so
I'd rather fix the issue than dismiss it in HEAD.
Thanks for the commit, Tom.
best regards,
Ranier Vilela