Message-ID: <34ff6307-d5be-d008-56ed-6249f3cd0749@dunslane.net>
Date: Thu, 17 Aug 2023 10:27:36 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: BUG #17994: Invalidating relcache corrupts tupDesc inside
 ExecEvalFieldStoreDeForm()
Content-Language: en-US
To: Alexander Lakhin <exclusion@gmail.com>, Andres Freund
 <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
References: <cf24e075-0809-b9b8-4c73-48eed105b9a9@dunslane.net>
 <1749275.1688132604@sss.pgh.pa.us>
 <e56b9c00-291b-dac0-6260-74137fdaffa5@dunslane.net>
 <1800418.1688152085@sss.pgh.pa.us>
 <3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net>
 <20230702215420.mal2kg5a4kasme5j@awork3.anarazel.de>
 <213081.1688335023@sss.pgh.pa.us>
 <20230702221320.t2yr52ozuuzmhcdy@awork3.anarazel.de>
 <2649715f-eff4-efe3-6a8d-7126a8624bc3@dunslane.net>
 <133548e7-9d69-4b74-5b11-d0ab30a616ef@dunslane.net>
 <20230710195107.r4s3ig6llhy2j3zb@awork3.anarazel.de>
 <27522775-aa59-38d4-716e-c2de35bc3d5a@dunslane.net>
 <0ac465e9-f6e2-4927-5aa9-a45ed6ce3801@dunslane.net>
 <b8f458d6-f0d5-3b0b-1f53-a51aec8aa15b@gmail.com>
From: Andrew Dunstan <andrew@dunslane.net>
In-Reply-To: <b8f458d6-f0d5-3b0b-1f53-a51aec8aa15b@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/34ff6307-d5be-d008-56ed-6249f3cd0749%40dunslane.net>
Precedence: bulk


On 2023-07-15 Sa 03:00, Alexander Lakhin wrote:
> Hello,
>
> 28.06.2023 21:49, Tom Lane wrote:
>> The bad news is that while investigating this I came across
>> another hazard that seems much messier to fix.  To wit, if
>> you have a tuple with "missing" pass-by-ref columns, then
>> some of the columns output by heap_deform_tuple() will
>> contain pointers into the tupdesc (cf. getmissingattr()).
>> That's not sufficient lifespan in the scenario we're dealing
>> with here: if the tupdesc gets trashed anytime before the
>> eventual ExecEvalFieldStoreForm(), we'll have garbage in the
>> result tuple.
>
> Please excuse me, as I'm definitely late to the party, but may I ask some
> questions to understand the issue discussed? I still can't see a 
> practical
> way to get garbage data in a tuple with missing columns, so maybe I'm
> missing something (except for columns).
>
> If we talk about ExecEvalFieldStoreDeForm() -> heap_deform_tuple() ->
> getmissingattr(), then I can't find a way to add a non-null default for a
> record type to exploit this path. The rowtypes test contains:
> -- at the moment this will not work due to ALTER TABLE inadequacy:
> alter table fullname add column suffix text default '';
> ERROR:  cannot alter table "fullname" because column "people.fn" uses 
> its row type
>
> So it seems to me, that ExecEvalFieldStoreDeForm() can't be called for a
> record/tuple having missing non-null attributes. And even if ALTER 
> TABLE was
> "adequate", does it mean that ExecEvalFieldStoreDeForm() would return 
> default
> values from the underlying table type?
>
>> Worse, I fear there may be many other places with latent bugs of the
>> same ilk.  Before the attmissingval code was added, one could assume
>> that the result of heap_deform_tuple would hold good as long as you
>> had a pin on the source tuple.  But now it depends on metadata as
>> well, and I don't think we have a coherent story about that.
>
> But is it true, that having a pin on tupdesc is enough to make sure that
> the result of heap_deform_tuple() holds good?
>
> If so, then probably we should find places, where tupdesc gets unpinned
> before that result is saved somewhere.
> I see the following callers of the heap_deform_tuple():
> src/backend/replication/logical/reorderbuffer.c
>     ReorderBufferToastReplace(): heap_deform_tuple() followed by 
> heap_form_tuple()
> src/backend/executor/execTuples.c
>     ExecForceStoreHeapTuple(), ExecForceStoreMinimalTuple(), 
> ExecStoreHeapTupleDatum(): use long-living and hopefully pinned 
> slot->tts_tupleDescriptor
> src/backend/executor/execExprInterp.c
>     ExecEvalFieldStoreDeForm(): the above question applies here
> src/backend/executor/spi.c
>     SPI_modifytuple(): heap_deform_tuple() followed by heap_form_tuple()
> src/backend/utils/adt/rowtypes.c
>     record_out(), hash_record(), hash_record_extended(): tupdesc 
> released after extracting/processing data
>     record_send(): tupdesc released after sending data
>     record_cmp(), record_eq(), record_image_cmp(), record_image_eq(): 
> tupdesc1. tupdesc2 released after comparing data
> src/backend/utils/adt/jsonfuncs.c
>     populate_record(): heap_deform_tuple() followed by heap_form_tuple()
> src/backend/utils/adt/expandedrecord.c
>     deconstruct_expanded_record() uses long-living (pinned) 
> erh->er_tupdesc
> src/backend/utils/adt/ri_triggers.c
>     RI_Initial_Check(), RI_PartitionRemove_Check(): use long-living? 
> SPI_tuptable->tupdesc
> src/backend/access/heap/heapam_handler.c
>     reform_and_rewrite_tuple(): uses RelationGetDescr(OldHeap) 
> (pinned, I suppose)
> src/backend/access/heap/heaptoast.c
>     heap_toast_delete(), heap_toast_insert_or_update() use long-living 
> rel->rd_att;
>     toast_flatten_tuple(): heap_deform_tuple(..., tupleDesc, ...) 
> followed by heap_form_tuple(..., tupleDesc, ...)
>     toast_flatten_tuple_to_datum(): heap_deform_tuple followed by 
> detoasting attrs
> src/backend/access/heap/heapam.c
>     ExtractReplicaIdentity(): heap_deform_tuple(..., desc, ...) 
> followed by heap_form_tuple(..., desc, ...)
> src/backend/access/common/heaptuple.c
>     heap_modify_tuple(): heap_deform_tuple(..., tupleDesc, ...) 
> followed by heap_form_tuple(..., tupleDesc, ...)
>     heap_modify_tuple_by_cols(): heap_deform_tuple(..., tupleDesc, 
> ...) followed by heap_form_tuple(..., tupleDesc, ...)
> src/backend/access/common/tupconvert.c
>     execute_attr_map_tuple(): heap_deform_tuple() followed by 
> heap_form_tuple()
> src/test/regress/regress.c
>     make_tuple_indirect(): heap_deform_tuple() followed by 
> heap_form_tuple()
> src/pl/plpgsql/src/pl_exec.c
>     exec_move_row() called with tupdesc = NULL, SPI_tuptable->tupdesc, 
> or tupdesc released after the call
> contrib/hstore/hstore_io.c
>     hstore_from_record(), hstore_populate_record(): tupdesc released 
> after extracting data
>
> And even if I missed some possibly problematic calls, maybe it's worth to
> consider fixing exactly that places...
>
> Also, besides heap_deform_tuple(), getmissingattr() is called from
> heap_getattr(). And heap_getattr() is used in many places, but most of 
> them
> are protected too due to tupdesc pinned.
> Two suspicious places that I found are GetAttributeByName() and
> GetAttributeByNum(), so when using these functions you can really get the
> missing attribute value with the tupdesc released. But what 
> circumstances do
> we need to end up with an invalid data?
> 1) Write a function that will call GetAttributeByName() and hold it's
>  result inside for some time.
> 2) Add a passed-by-ref column with a default value to a table.
> 3) Pass to that function a tuple without the "missing" column (just
>  "SELECT func(table) FROM table" won't do, but func(OLD) in a trigger 
> should).
> 4) Trigger a relcache invalidation.
> 5) Perform a database access inside that function to process the 
> invalidation.
> 6) Access the result of GetAttributeByName() after that.
>
> Maybe we could construct such test case, e.g. add to pg_regress one more
> SQL-accessible C function, bu9ccvhhihvcnb·t I wonder, is this the way 
> to go?
>
> On the other hand, probably there are extensions, that use
> GetAttributeByName() in the non-safe way (as the function documentation
> doesn't warn about such issues), but maybe just perform datumCopy inside
> that function (and similar one(s))?
> Though, perhaps I just don't notice the elephant in the core...


I was waiting to see if others had a reaction to this, but ...

I think the last point (possible unsafe use by extensions) is reason 
enough to proceed with the proposed mitigation, which is pretty simple 
and non-invasive..


cheers


andrew

-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com