Use-after-free in ER_get_flat_size() when a PL/pgSQL expression references a TOASTed field and the whole record

Hi hackers,

We hit a backend crash (SIGSEGV) in PL/pgSQL. It's a use-after-free in
`ER_get_flat_size()` (`src/backend/utils/adt/expandedrecord.c`). The trigger
is a statement that references a TOASTed field of a composite variable and
also the whole variable, for example:

```sql
INSERT INTO snap (big, whole) VALUES (p.big, to_jsonb(p));
```

The two attached patches add a regression test and fix the bug.
The issue
`p` is an expanded record stored as separate field values, and `p.big` is
held as an out-of-line TOAST pointer. The sequence is:

1.
`p.big` is evaluated first and returns the field datum by reference. The
*
pointer points into the record's own storage.
2.
`to_jsonb(p)` needs a flat tuple, so `ER_get_flat_size()` detoasts the
*
external field in place through `expanded_record_set_field_internal()`.
That function pfrees the old field value whenever it isn't part of the
record's flat tuple. Here the record has no flat tuple (`fvalue == NULL`),
so the old TOAST pointer, which step 1 still holds, gets freed.
3.
The statement then uses the dangling pointer from step 1.

Ordinary field assignments are right to free the old value. Flattening is
different, because callers treat it as a read-only operation on the record.
Three conditions are needed to get the record into this form:

*
The field is stored out of line (for example, `STORAGE EXTERNAL` with a
*
large value).
*
The row type has a dropped column. That makes `SELECT INTO` / `FOR ... IN
*
SELECT` store the fields separately instead of keeping a flat tuple.
*
One expression or target list references both the field and the whole
*

The calls involved date from commit 4b93f57999a (2018, PG 11). The same code
is present in master and in REL_13_STABLE through REL_18_STABLE.
Steps to reproduce
We found this on REL_18_STABLE (18.6, cc4bf6e9acc). The build was
`--enable-cassert`, so freed memory is overwritten (`CLOBBER_FREED_MEMORY`)
and the failure is deterministic.
Wrong result, with no crash:

```sql

create table toasted_hole (id int, junk text, b text);
alter table toasted_hole alter column b set storage external;
insert into toasted_hole values (1, 'x', repeat('ffoob', 4000));
alter table toasted_hole drop column junk;
create function toasted_hole_test() returns bool language plpgsql as
$$ declare r toasted_hole;
begin
select * into r from toasted_hole;
return r.b = (to_jsonb(r) ->> 'b');
end $$;
select toasted_hole_test(); -- returns f; expected t
```

Crash. This is the shape of the original report: an AFTER trigger loops
over the table and passes each row to a function that inserts the field and
the whole record. The INSERT is planned as a custom plan for its first five
executions, and a custom plan replaces `p.big` with a Const copy of the
value. So the crash only shows up once the generic plan is used.
`force_generic_plan` makes it happen on the first execution:

```sql
create table src (id int, junk text, b text);
alter table src alter column b set storage external;
insert into src values (1, 'x', repeat('ffoob', 4000));
alter table src drop column junk;
create table snap (big text, whole jsonb);
create table driver (id int primary key, v int not null default 0);
insert into driver values (1);
create function snapshot(p src) returns void language plpgsql as
$$ begin
insert into snap (big, whole) values (p.b, to_jsonb(p));
end $$;
create function bump() returns trigger language plpgsql as
$$ declare r src;
begin
for r in select s.* from src s loop
perform snapshot(r);
end loop;
return null;
end $$;
create trigger driver_au after update on driver
for each row execute function bump();
set plan_cache_mode = force_generic_plan;
update driver set v = v + 1 where id = 1; -- backend crashes
```

Backtrace from the core file (excerpt):

```
#0 __memmove_avx_unaligned_erms () from /lib64/libc.so.6
#1 toast_save_datum (rel=..., value=891182572, oldexternal=0x0, options=0) at toast_internals.c:323
#2 toast_tuple_externalize (ttc=..., attribute=1, options=0) at toast_helper.c:263
#3 heap_toast_insert_or_update (...) at heaptoast.c:234
#4 heap_prepare_insert (...) at heapam.c:2337
#5 heap_insert (...) at heapam.c:2106
...
#8 ExecInsert (...) at nodeModifyTable.c:1234
#9 ExecModifyTable (...) at nodeModifyTable.c:4524
...
#15 _SPI_pquery (...) at spi.c:2932
...
#18 exec_stmt_execsql (...) at pl_exec.c:4287
```

`value` in frame #1 is the stale datum from `p.b`. The crash site moves around
depending on how the freed memory is reused. We have also seen it in
`slot_deform_heap_tuple_internal`, reading a corrupt tuple length of about
100 MB, and in `heap_fill_tuple`/`fill_val` under `ExecCopySlot`.
The patch
My colleague Xnerv Wang has a patch that contains the following.

*
test.patch adds the wrong-result case above to `plpgsql_record.sql`. Without
*
0002 it fails with `f` instead of `t`.
*
code.patch is the fix. `expanded_record_set_field_internal()` becomes a thin
*
wrapper around a new static `expanded_record_set_field_worker()`, which
takes a `free_old_value` flag. `ER_get_flat_size()` calls the worker with
`free_old_value = false`, so detoasting during flattening no longer frees
the old value. That old value is only a small TOAST pointer in the record's
memory context, so it stays allocated until the record is deleted. All
other callers keep the existing behavior.

The one cost is that a TOAST pointer per detoasted field stays allocated for
the record's lifetime. That seemed better to us than making callers copy
field values defensively, or making flattening build a separate tuple without
updating `dvalues[]`. We're open to other approaches.
The patches apply cleanly to master and REL_18_STABLE.
Testing
All of this was on REL_18_STABLE with `--enable-cassert --enable-debug -O0`:

*
Without the fix: the wrong-result test returns `f`. The trigger
*
reproducer crashes the backend with signal 11 every time. We confirmed it
with both `make check` and `make installcheck`, and examined the core file.
*
With the fix: both cases pass and there are no crashes.
*
`make -C src/pl/plpgsql/src check` and `installcheck` pass (all 14 tests,
*
including the crash reproducer above, which we ran as an extra local test).
*
`make check-world` passes.

Thoughts and review welcome.

Thanks,
Kristian Lejao