Horton

Thread

Commits

  1. Remove COMMAND_TAG_NEXTTAG from enum CommandTag.

  1. Fix out-of-bounds in the function GetCommandTagName

    Ranier Vilela <ranier.vf@gmail.com> — 2024-04-14T23:17:35Z

    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

  2. Re: Fix out-of-bounds in the function GetCommandTagName

    David Rowley <dgrowleyml@gmail.com> — 2024-04-14T23:38:09Z

    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

  3. Re: Fix out-of-bounds in the function GetCommandTagName

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-14T23:54:28Z

    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

  4. Re: Fix out-of-bounds in the function GetCommandTagName

    Ranier Vilela <ranier.vf@gmail.com> — 2024-04-15T00:12:31Z

    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

  5. Re: Fix out-of-bounds in the function GetCommandTagName

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T01:09:02Z

    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

  6. Re: Fix out-of-bounds in the function GetCommandTagName

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-15T01:15:49Z

    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

  7. Re: Fix out-of-bounds in the function GetCommandTagName

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T02:12:34Z

    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

  8. Re: Fix out-of-bounds in the function GetCommandTagName

    Ranier Vilela <ranier.vf@gmail.com> — 2024-04-15T11:35:55Z

    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

  9. Re: Fix out-of-bounds in the function GetCommandTagName

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-13T17:38:16Z

    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

  10. Re: Fix out-of-bounds in the function GetCommandTagName

    Ranier Vilela <ranier.vf@gmail.com> — 2024-05-13T18:02:05Z

    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