Commit 55566c9a authored by Heikki Linnakangas's avatar Heikki Linnakangas

Fix dangling smgr_owner pointer when a fake relcache entry is freed.

A fake relcache entry can "own" a SmgrRelation object, like a regular
relcache entry. But when it was free'd, the owner field in SmgrRelation
was not cleared, so it was left pointing to free'd memory.

Amazingly this apparently hasn't caused crashes in practice, or we would've
heard about it earlier. Andres found this with Valgrind.

Report and fix by Andres Freund, with minor modifications by me. Backpatch
to all supported versions.
parent ad7b48ea
...@@ -445,6 +445,9 @@ CreateFakeRelcacheEntry(RelFileNode rnode) ...@@ -445,6 +445,9 @@ CreateFakeRelcacheEntry(RelFileNode rnode)
void void
FreeFakeRelcacheEntry(Relation fakerel) FreeFakeRelcacheEntry(Relation fakerel)
{ {
/* make sure the fakerel is not referenced by the SmgrRelation anymore */
if (fakerel->rd_smgr != NULL)
smgrclearowner(&fakerel->rd_smgr, fakerel->rd_smgr);
pfree(fakerel); pfree(fakerel);
} }
......
...@@ -84,6 +84,7 @@ static SMgrRelation first_unowned_reln = NULL; ...@@ -84,6 +84,7 @@ static SMgrRelation first_unowned_reln = NULL;
/* local function prototypes */ /* local function prototypes */
static void smgrshutdown(int code, Datum arg); static void smgrshutdown(int code, Datum arg);
static void add_to_unowned_list(SMgrRelation reln);
static void remove_from_unowned_list(SMgrRelation reln); static void remove_from_unowned_list(SMgrRelation reln);
...@@ -174,9 +175,8 @@ smgropen(RelFileNode rnode, BackendId backend) ...@@ -174,9 +175,8 @@ smgropen(RelFileNode rnode, BackendId backend)
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
reln->md_fd[forknum] = NULL; reln->md_fd[forknum] = NULL;
/* place it at head of unowned list (to make smgrsetowner cheap) */ /* it has no owner yet */
reln->next_unowned_reln = first_unowned_reln; add_to_unowned_list(reln);
first_unowned_reln = reln;
} }
return reln; return reln;
...@@ -191,7 +191,7 @@ smgropen(RelFileNode rnode, BackendId backend) ...@@ -191,7 +191,7 @@ smgropen(RelFileNode rnode, BackendId backend)
void void
smgrsetowner(SMgrRelation *owner, SMgrRelation reln) smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
{ {
/* We don't currently support "disowning" an SMgrRelation here */ /* We don't support "disowning" an SMgrRelation here, use smgrclearowner */
Assert(owner != NULL); Assert(owner != NULL);
/* /*
...@@ -213,6 +213,40 @@ smgrsetowner(SMgrRelation *owner, SMgrRelation reln) ...@@ -213,6 +213,40 @@ smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
*owner = reln; *owner = reln;
} }
/*
* smgrclearowner() -- Remove long-lived reference to an SMgrRelation object
* if one exists
*/
void
smgrclearowner(SMgrRelation *owner, SMgrRelation reln)
{
/* Do nothing if the SMgrRelation object is not owned by the owner */
if (reln->smgr_owner != owner)
return;
/* unset the owner's reference */
*owner = NULL;
/* unset our reference to the owner */
reln->smgr_owner = NULL;
add_to_unowned_list(reln);
}
/*
* add_to_unowned_list -- link an SMgrRelation onto the unowned list
*
* Check remove_from_unowned_list()'s comments for performance
* considerations.
*/
static void
add_to_unowned_list(SMgrRelation reln)
{
/* place it at head of the list (to make smgrsetowner cheap) */
reln->next_unowned_reln = first_unowned_reln;
first_unowned_reln = reln;
}
/* /*
* remove_from_unowned_list -- unlink an SMgrRelation from the unowned list * remove_from_unowned_list -- unlink an SMgrRelation from the unowned list
* *
......
...@@ -80,6 +80,7 @@ extern void smgrinit(void); ...@@ -80,6 +80,7 @@ extern void smgrinit(void);
extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend); extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend);
extern bool smgrexists(SMgrRelation reln, ForkNumber forknum); extern bool smgrexists(SMgrRelation reln, ForkNumber forknum);
extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln); extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln);
extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln);
extern void smgrclose(SMgrRelation reln); extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void); extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode); extern void smgrclosenode(RelFileNodeBackend rnode);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment