Thread: BUG #14220: pg_get_expr() with an incorrect relation id crashes the server

VGhlIGZvbGxvd2luZyBidWcgaGFzIGJlZW4gbG9nZ2VkIG9uIHRoZSB3ZWJz
aXRlOgoKQnVnIHJlZmVyZW5jZTogICAgICAxNDIyMApMb2dnZWQgYnk6ICAg
ICAgICAgIENocmlzIEhhbmtzCkVtYWlsIGFkZHJlc3M6ICAgICAgY2hyaXN0
b3BoZXIubS5oYW5rc0BnbWFpbC5jb20KUG9zdGdyZVNRTCB2ZXJzaW9uOiA5
LjUuMwpPcGVyYXRpbmcgc3lzdGVtOiAgIExpbnV4IE1pbnQgMTcuMiA2NC1i
aXQKRGVzY3JpcHRpb246ICAgICAgICAKClNFTEVDVCB2ZXJzaW9uKCk6DQoi
UG9zdGdyZVNRTCA5LjUuMyBvbiB4ODZfNjQtcGMtbGludXgtZ251LCBjb21w
aWxlZCBieSBnY2MgKFVidW50dQo0LjguMi0xOXVidW50dTEpIDQuOC4yLCA2
NC1iaXQiDQoNClJlcHJvZHVjdGlvbjoNCg0KQ1JFQVRFIFRBQkxFICJ0ZXN0
X3RhYmxlIiAoImlkIiBzZXJpYWwgUFJJTUFSWSBLRVksICJjb2x1bW5fMSIg
aW50NCBOT1QKTlVMTCwgImNvbHVtbl8yIiBpbnQ0KTsNCg0KQ1JFQVRFIElO
REVYICJpZHgiIE9OICJ0ZXN0X3RhYmxlIiAoImNvbHVtbl8xIikgV0hFUkUg
KCJjb2x1bW5fMiIgPSA0KTsNCg0KU0VMRUNUIHBnX2dldF9leHByKGkuaW5k
cHJlZCwgaS5pbmRleHJlbGlkKSBGUk9NIHBnX2luZGV4IGk7DQoNCkkgcmVh
bGl6ZSB0aGF0IHRoZSBxdWVyeSBpcyBpbmNvcnJlY3QsIGluIG9yZGVyIHRv
IHJldHJpZXZlIHRoZSBmaWx0ZXIKZXhwcmVzc2lvbiBhcyBTUUwgSSBuZWVk
IHRvIGNhbGwgcGdfZ2V0X2V4cHIoaS5pbmRwcmVkLCBpLmluZHJlbGlkKS4g
QnV0IGl0CnNlZW1zIGxpa2UgaXQgc2hvdWxkIHJhaXNlIGFuIGVycm9yLCBy
YXRoZXIgdGhhbiBjYXVzaW5nIHRoZSBzZXJ2ZXIgdG8KY3Jhc2guCgo=

On Thu, Jun 30, 2016 at 10:43 AM,  <christopher.m.hanks@gmail.com> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      14220
> Logged by:          Chris Hanks
> Email address:      christopher.m.hanks@gmail.com
> PostgreSQL version: 9.5.3
> Operating system:   Linux Mint 17.2 64-bit
> Description:
>
> SELECT version():
> "PostgreSQL 9.5.3 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
> 4.8.2-19ubuntu1) 4.8.2, 64-bit"
>
> Reproduction:
>
> CREATE TABLE "test_table" ("id" serial PRIMARY KEY, "column_1" int4 NOT
> NULL, "column_2" int4);
>
> CREATE INDEX "idx" ON "test_table" ("column_1") WHERE ("column_2" = 4);
>
> SELECT pg_get_expr(i.indpred, i.indexrelid) FROM pg_index i;
>
> I realize that the query is incorrect, in order to retrieve the filter
> expression as SQL I need to call pg_get_expr(i.indpred, i.indrelid). But it
> seems like it should raise an error, rather than causing the server to
> crash.

Maybe the attnum bounds check should be an error rather than an
assertion, like in the attached.  Thought perhaps with a better
message...

--
Thomas Munro
http://www.enterprisedb.com

On Fri, Jul 1, 2016 at 5:42 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Maybe the attnum bounds check should be an error rather than an
> assertion, like in the attached.  Thought perhaps with a better
> message...

Quite interesting report. It looks right to me to do so with an elog(ERROR).

+        if (attnum > colinfo->num_cols)
+            elog(ERROR, "cannot decompile bogus attnum: %d", attnum);
Why not "incorrect number of arguments: specified %d but found %d" instead?

I have poked at this bug report yesterday and today, and found that it
is as well possible to hit the second assertion two lines below with a
dropped column. So I would like to propose the patch attached to
replace both assertions with an elog(ERROR). Test cases are included,
though I am not sure if they bring much value.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> I have poked at this bug report yesterday and today, and found that it
> is as well possible to hit the second assertion two lines below with a
> dropped column. So I would like to propose the patch attached to
> replace both assertions with an elog(ERROR).

There's a quite similar error check just a few lines above that prints

        elog(ERROR, "bogus varattno for subquery var: %d", var->varattno);

I'm not necessarily wedded to that exact phrasing, but I think these new
error messages should be consistent with that one.  I'm a bit inclined
to make all three of them read like

        elog(ERROR, "invalid attnum %d for table \"%s\"",
             attnum, rte->eref->aliasname);

> Test cases are included,
> though I am not sure if they bring much value.

Yeah, I can't get very excited about memorializing those.

            regards, tom lane