Обсуждение: [PATCH] Fix memory corruption in pg_shdepend.c
Hi hackers,
One of our test runs under the memory sanitizer cathed [1] the
following stacktrace:
```
heaptuple.c:1044:13: runtime error: load of value 111, which is not a
valid value for type '_Bool'
#0 0x55fbb5e0857b in heap_form_tuple
/home/runner/pgbuild/src/backend/access/common/heaptuple.c:1044
#1 0x55fbb679f62d in tts_heap_materialize
/home/runner/pgbuild/src/backend/executor/execTuples.c:381
#2 0x55fbb67addcf in ExecFetchSlotHeapTuple
/home/runner/pgbuild/src/backend/executor/execTuples.c:1654
#3 0x55fbb5f8127d in heap_multi_insert
/home/runner/pgbuild/src/backend/access/heap/heapam.c:2330
#4 0x55fbb6261b50 in CatalogTuplesMultiInsertWithInfo
/home/runner/pgbuild/src/backend/catalog/indexing.c:268
#5 0x55fbb62ce5aa in copyTemplateDependencies
/home/runner/pgbuild/src/backend/catalog/pg_shdepend.c:933
#6 0x55fbb650eb98 in createdb
/home/runner/pgbuild/src/backend/commands/dbcommands.c:590
#7 0x55fbb7062b30 in standard_ProcessUtility
/home/runner/pgbuild/src/backend/tcop/utility.c:773
#8 0x7fa942a63c13 in loader_process_utility_hook
/home/runner/work/timescaledb/timescaledb/src/loader/loader.c:522
#9 0x55fbb7063807 in ProcessUtility
/home/runner/pgbuild/src/backend/tcop/utility.c:523
#10 0x55fbb705bac3 in PortalRunUtility
/home/runner/pgbuild/src/backend/tcop/pquery.c:1147
#11 0x55fbb705c6fe in PortalRunMulti
/home/runner/pgbuild/src/backend/tcop/pquery.c:1304
#12 0x55fbb705d485 in PortalRun
/home/runner/pgbuild/src/backend/tcop/pquery.c:786
#13 0x55fbb704f613 in exec_simple_query
/home/runner/pgbuild/src/backend/tcop/postgres.c:1214
#14 0x55fbb7054b30 in PostgresMain
/home/runner/pgbuild/src/backend/tcop/postgres.c:4486
#15 0x55fbb6d78551 in BackendRun
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:4506
#16 0x55fbb6d8334c in BackendStartup
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:4228
#17 0x55fbb6d840cd in ServerLoop
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:1745
#18 0x55fbb6d86611 in PostmasterMain
/home/runner/pgbuild/src/backend/postmaster/postmaster.c:1417
#19 0x55fbb6970b9b in main /home/runner/pgbuild/src/backend/main/main.c:209
```
It seems to be a bug in the PostgreSQL core. The memory corruption
happens @ pg_shdepend.c:914:
```
slot[slot_stored_count]->tts_values[Anum_pg_shdepend_refobjid
] = shdep->refobjid;
slot[slot_stored_count]->tts_values[Anum_pg_shdepend_deptype]
= shdep->deptype; <--- HERE
ExecStoreVirtualTuple(slot[slot_stored_count]);
```
The shdep->deptype value gets written to slot[0]->tts_isnull:
```
(lldb) p shdep->deptype
(char) $0 = 'o'
(lldb) p ((uint8_t*)slot[0]->tts_isnull)[0]
(uint8_t) $2 = 'o'
(lldb) p/d 'o'
(char) $4 = 111
```
I checked the rest of the PostgreSQL code and apparently, it should
have been tts_values[Anum_pg_shdepend_FOO - 1].
The patch is attached. The problem was first reported offlist by Sven
Klemm. Investigated and fixed by me.
[1]: https://github.com/timescale/timescaledb/actions/runs/1343346998
--
Best regards,
Aleksander Alekseev
Вложения
On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote: > I checked the rest of the PostgreSQL code and apparently, it should > have been tts_values[Anum_pg_shdepend_FOO - 1]. > > The patch is attached. The problem was first reported offlist by Sven > Klemm. Investigated and fixed by me. Yes, that's indeed a one-off bug when copying shared dependencies of a template database to the new one. This is new as of e3931d0, so I'll take care of that and double-check the area while on. -- Michael
Вложения
> On 20 Oct 2021, at 12:55, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote: >> I checked the rest of the PostgreSQL code and apparently, it should >> have been tts_values[Anum_pg_shdepend_FOO - 1]. >> >> The patch is attached. The problem was first reported offlist by Sven >> Klemm. Investigated and fixed by me. > > Yes, that's indeed a one-off bug when copying shared dependencies of a > template database to the new one. This is new as of e3931d0, so I'll > take care of that and double-check the area while on. The attached patch looks correct to me. Skimming the referenced commit I see nothing else sticking out. -- Daniel Gustafsson https://vmware.com/
On 2021-Oct-20, Michael Paquier wrote:
> On Wed, Oct 20, 2021 at 01:01:31PM +0300, Aleksander Alekseev wrote:
> > I checked the rest of the PostgreSQL code and apparently, it should
> > have been tts_values[Anum_pg_shdepend_FOO - 1].
> >
> > The patch is attached. The problem was first reported offlist by Sven
> > Klemm. Investigated and fixed by me.
>
> Yes, that's indeed a one-off bug when copying shared dependencies of a
> template database to the new one. This is new as of e3931d0, so I'll
> take care of that and double-check the area while on.
Ouch ... this means that pg_shdepends contents are broken for databases
created with 14.0? hmm ... yes.
alvherre=# create role rol;
CREATE ROLE
alvherre=# create table blarg() ;
CREATE TABLE
alvherre=# alter table blarg owner to rol;
ALTER TABLE
alvherre=# create database bar template alvherre;
CREATE DATABASE
alvherre=# \c bar
Ahora está conectado a la base de datos «bar» con el usuario «alvherre».
bar=# select * from pg_shdepend;
dbid | classid | objid | objsubid | refclassid | refobjid | deptype
-------+---------+-------+----------+------------+----------+---------
0 | 0 | 0 | 0 | 1260 | 10 | p
0 | 0 | 0 | 0 | 1260 | 6171 | p
0 | 0 | 0 | 0 | 1260 | 6181 | p
0 | 0 | 0 | 0 | 1260 | 6182 | p
0 | 0 | 0 | 0 | 1260 | 3373 | p
0 | 0 | 0 | 0 | 1260 | 3374 | p
0 | 0 | 0 | 0 | 1260 | 3375 | p
0 | 0 | 0 | 0 | 1260 | 3377 | p
0 | 0 | 0 | 0 | 1260 | 4569 | p
0 | 0 | 0 | 0 | 1260 | 4570 | p
0 | 0 | 0 | 0 | 1260 | 4571 | p
0 | 0 | 0 | 0 | 1260 | 4200 | p
12975 | 1259 | 37686 | 0 | 1260 | 37685 | o
| 37689 | 1259 | 37686 | 0 | 1260 | 5
(14 filas)
bar=# select 37689::regclass;
regclass
----------
37689
(1 fila)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Wed, Oct 20, 2021 at 09:19:51AM -0300, Alvaro Herrera wrote: > Ouch ... this means that pg_shdepends contents are broken for databases > created with 14.0? hmm ... yes. Yes, it means so :( I have fixed the issue for now, and monitored the rest of the tree. Another issue is that we have zero coverage for this area of the code when creating a database from a template and copying over shared dependencies: https://coverage.postgresql.org/src/backend/catalog/pg_shdepend.c.gcov.html It is easy enough to get an error on the new database with pg_describe_object(). Your part about adding a shared dependency with a table on a given role is simple enough, as well. While looking for a place where to put such a test, 020_createdb.pl felt like a natural place and we don't have any coverage for the case of TEMPLATE with createdb. So I would like to suggest something like the attached for HEAD. -- Michael
Вложения
On Wed, Oct 20, 2021 at 5:20 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Ouch ... this means that pg_shdepends contents are broken for databases > created with 14.0? hmm ... yes. I think that EDB's pg_catcheck tool can detect problems like this one. Perhaps it can be converted into an amcheck/pg_amcheck patch, and submitted. That would give us very broad coverage. -- Peter Geoghegan
On Wed, Oct 20, 2021 at 07:59:50PM -0700, Peter Geoghegan wrote: > I think that EDB's pg_catcheck tool can detect problems like this one. Yes, pg_catcheck is able to catch that. > Perhaps it can be converted into an amcheck/pg_amcheck patch, and > submitted. That would give us very broad coverage. Perhaps. This means the creation of a new database with shared deps in contrib/amcheck/t/. But is amcheck really a correct target here? The fields involved here are an int, some OIDs and a char with a given subset of values making them harder to check. pg_catcheck does checks across catalogs, maintaining a mapping list as of its definitions.c. -- Michael
Вложения
On Wed, Oct 20, 2021 at 8:27 PM Michael Paquier <michael@paquier.xyz> wrote: > Perhaps. This means the creation of a new database with shared deps > in contrib/amcheck/t/. But is amcheck really a correct target here? > The fields involved here are an int, some OIDs and a char with a given > subset of values making them harder to check. pg_catcheck does checks > across catalogs, maintaining a mapping list as of its definitions.c. Users should be able to use pg_amcheck as a high-level corruption detection tool, which should include any new pg_catcheck style catalog checking functionality. Whether or not we need to involve contrib/amcheck itself doesn't seem important to me right now. Offhand I think that we wouldn't, because as you point out pg_catcheck is a frontend program that checks multiple databases. -- Peter Geoghegan
On 2021-Oct-21, Michael Paquier wrote: > On Wed, Oct 20, 2021 at 09:19:51AM -0300, Alvaro Herrera wrote: > > Ouch ... this means that pg_shdepends contents are broken for databases > > created with 14.0? hmm ... yes. > > Yes, it means so :( For the upcoming release notes in 14.1 I think we'd do well to document how to find out if you're affected by this; and if you are, how to fix it. I suppose pg_describe_object can be used on the contents of pg_shdepend to detect it. I'm less sure what to do to correct it -- delete the bogus entries and regenerate them with some bulk query? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I suppose pg_describe_object can be used on the contents of pg_shdepend
> to detect it. I'm less sure what to do to correct it -- delete the
> bogus entries and regenerate them with some bulk query?
Seems that what copyTemplateDependencies wants to do can easily be
modeled by a SQL query, assuming you know which DB was cloned to
which other one, and that the source's shdeps didn't change since
then. However, I'm not sure how we can get rid of existing bogus
entries, especially if we'd like to preserve not-bogus ones
(which very likely have gotten added to the destination DB since
it was created).
On the whole I'm afraid that people messing with this manually are
likely to do more harm than good. pg_shdepend entries that don't
match any object probably won't cause a problem, and the lack of
protection against untimely dropping a role is unlikely to be much
of an issue for a role you're referencing in a template database.
So I suspect that practical issues will be rare. We're fortunate
that cloning a nonempty template database is rare already.
BTW, I think there is an additional bug in copyTemplateDependencies:
I do not see it initializing slot->tts_isnull[] anywhere. It
probably accidentally works (at least in devel builds) because we zero
that memory somewhere else, but surely this code shouldn't assume that?
regards, tom lane
On Thu, Oct 21, 2021 at 8:52 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We're fortunate
that cloning a nonempty template database is rare already.
That, and a major use case for doing so is to quickly stage up testing data in a new database (i.e., not a production use case). Though I could see tenant-based products using this to bootstrap new clients I'd hope that is a minority case.
David J.
Hi Tom, > BTW, I think there is an additional bug in copyTemplateDependencies: > I do not see it initializing slot->tts_isnull[] anywhere. It > probably accidentally works (at least in devel builds) because we zero > that memory somewhere else, but surely this code shouldn't assume that? tts_isnull[] is zeroed in: - copyTemplateDependencies -- MakeSingleTupleTableSlot, which simply wraps: --- MakeTupleTableSlot ... where the slot is allocated with palloc0. The assumption that MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed tts_isnull[] seems reasonable, no? What confuses me is the fact that we have two procedures that do the same thing. Maybe one is redundant. -- Best regards, Aleksander Alekseev
On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote: > ... where the slot is allocated with palloc0. The assumption that > MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed > tts_isnull[] seems reasonable, no? Yes, I don't see any need to do something more here. The number of arguments is fetched from the tuple descriptor itself, so the allocation is sufficient. > What confuses me is the fact that we have two procedures that do the > same thing. Maybe one is redundant. Do you have something in mind here? Taking advantage of the catalog types and knowing that this is a one-off, it is possible to recover dbid, classid, objid, objsubid and refclassid. deptype can be mostly guessed from refclassid, but the real problem is that refobjid is just lost because of the casting to a char from and Oid. [ ... Thinks more ... ] Hmm. Wouldn't it be as simple as removing the entries in pg_shdepend where dbid is NULL, and do an INSERT/SELECT with the existing entries in pg_shdepend from the template database, updating dbid to the new database? That would require users to know which template they used as origin, as well as we could assume that no shared deps have changed but that can be guessed by looking at all the misplaced fields. It is true enough that users could make a lot of damage with chirurgy DMLs on catalogs, though. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote:
>> ... where the slot is allocated with palloc0. The assumption that
>> MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed
>> tts_isnull[] seems reasonable, no?
> Yes, I don't see any need to do something more here.
That assumption is exactly what I'm objecting to. I don't think
we make it in other places, and I don't like making it here.
(By "here" I mean all of e3931d0, because it made the same omission
in several places.)
The primary reason why I think it's a bad idea is that only one
path in MakeSingleTupleTableSlot provides a pre-zeroed tts_isnull
array --- if you don't supply a tuple descriptor at creation,
the assumption falls down. So even if this coding technique is
safe where it is, it is a hazard for anyone copying the code into
some other context.
I might be happier if we tried to guarantee that *every* way of
creating a slot will end with a pre-zeroed isnull array, and then
got rid of any thereby-duplicative memsets. But that would be
a lot more invasive than just making these places get in step.
regards, tom lane
Hi Michael, > Do you have something in mind here? Yep. This is not a priority though, thus I created a separate CF entry: https://commitfest.postgresql.org/35/3371/ -- Best regards, Aleksander Alekseev
> On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Michael Paquier <michael@paquier.xyz> writes: >> On Fri, Oct 22, 2021 at 10:48:57AM +0300, Aleksander Alekseev wrote: >>> ... where the slot is allocated with palloc0. The assumption that >>> MakeSingleTupleTableSlot() returns valid TupleTableSlot* with zeroed >>> tts_isnull[] seems reasonable, no? > >> Yes, I don't see any need to do something more here. > > That assumption is exactly what I'm objecting to. I don't think > we make it in other places, and I don't like making it here. > (By "here" I mean all of e3931d0, because it made the same omission > in several places.) The attached fixes the the two ones I spotted, are there any I missed? Regardless of if we want to change the API (as discussed elsewhere here and in a new thread), something like the attached should be done first and in 14 I think. -- Daniel Gustafsson https://vmware.com/
Вложения
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (By "here" I mean all of e3931d0, because it made the same omission
>> in several places.)
> The attached fixes the the two ones I spotted, are there any I missed?
Ah, you're right, InsertPgAttributeTuples is the only other spot in
that patch that's actually touching slots. I'd skimmed it a little
too quickly.
regards, tom lane
> On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >>> On 22 Oct 2021, at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> (By "here" I mean all of e3931d0, because it made the same omission >>> in several places.) > >> The attached fixes the the two ones I spotted, are there any I missed? > > Ah, you're right, InsertPgAttributeTuples is the only other spot in > that patch that's actually touching slots. I'd skimmed it a little > too quickly. Thanks for confirming, unless there are objections I'll apply the fix to master and backpatch to 14. -- Daniel Gustafsson https://vmware.com/
On Fri, Oct 22, 2021 at 10:49:38PM +0200, Daniel Gustafsson wrote: > On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Ah, you're right, InsertPgAttributeTuples is the only other spot in >> that patch that's actually touching slots. I'd skimmed it a little >> too quickly. > > Thanks for confirming, unless there are objections I'll apply the fix to master > and backpatch to 14. Fine by me. The patch looks OK. -- Michael
Вложения
On Thu, Oct 21, 2021 at 11:42:31AM +0900, Michael Paquier wrote: > It is easy enough to get an error on the new database with > pg_describe_object(). Your part about adding a shared dependency with > a table on a given role is simple enough, as well. While looking for > a place where to put such a test, 020_createdb.pl felt like a natural > place and we don't have any coverage for the case of TEMPLATE with > createdb. So I would like to suggest something like the attached for > HEAD. I was thinking on this one over the last couple of days, and doing a copy of shared deps from a template within createdb still feels natural, as of this patch: https://www.postgresql.org/message-id/YXDTl+PfSnqmbbkE@paquier.xyz Would somebody object to the addition of this test? Or perhaps somebody has a better idea? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > I was thinking on this one over the last couple of days, and doing a > copy of shared deps from a template within createdb still feels > natural, as of this patch: > https://www.postgresql.org/message-id/YXDTl+PfSnqmbbkE@paquier.xyz > Would somebody object to the addition of this test? Or perhaps > somebody has a better idea? I agree that we're not testing that area well enough. Proposed patch seems basically OK, but I think the test needs to be stricter about what the expected output looks like --- for instance, it wouldn't complain if tab_foobar were described as something other than a table. regards, tom lane
On Mon, Oct 25, 2021 at 11:59:52AM -0400, Tom Lane wrote: > I agree that we're not testing that area well enough. Proposed > patch seems basically OK, but I think the test needs to be stricter > about what the expected output looks like --- for instance, it > wouldn't complain if tab_foobar were described as something other > than a table. Indeed. There was also a problem in the regex itself, where '|' was not escaped so the regex was not strict enough. While on it, I have added a policy in the set copied to the new database. Testing the case where the set of slots is full would require 2300~ entries, that would take some time.. -- Michael
Вложения
> On 23 Oct 2021, at 00:47, Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Oct 22, 2021 at 10:49:38PM +0200, Daniel Gustafsson wrote: >> On 22 Oct 2021, at 20:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Ah, you're right, InsertPgAttributeTuples is the only other spot in >>> that patch that's actually touching slots. I'd skimmed it a little >>> too quickly. >> >> Thanks for confirming, unless there are objections I'll apply the fix to master >> and backpatch to 14. > > Fine by me. The patch looks OK. Applied to master and 14, thanks. -- Daniel Gustafsson https://vmware.com/
On Tue, Oct 26, 2021 at 02:43:26PM +0900, Michael Paquier wrote: > Indeed. There was also a problem in the regex itself, where '|' was > not escaped so the regex was not strict enough. While on it, I have > added a policy in the set copied to the new database. Testing the > case where the set of slots is full would require 2300~ entries, that > would take some time.. Applied this one as of 70bfc5a. -- Michael