Обсуждение: [MASSMAIL]Fix out-of-bounds in the function GetCommandTagName

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

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

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

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