Re: shared-memory based stats collector

Hi,

On 2021-03-10 20:26:56 -0800, Andres Freund wrote:
> > + shhashent = dshash_find_extended(pgStatSharedHash, &key,
> > + create, nowait, create, &shfound);
> > + if (shhashent)
> > + {
> > + if (create && !shfound)
> > + {
> > + /* Create new stats entry. */
> > + dsa_pointer chunk = dsa_allocate0(area,
> > + pgstat_sharedentsize[type]);
> > +
> > + shheader = dsa_get_address(area, chunk);
> > + LWLockInitialize(&shheader->lock, LWTRANCHE_STATS);
> > + pg_atomic_init_u32(&shheader->refcount, 0);
> > +
> > + /* Link the new entry from the hash entry. */
> > + shhashent->body = chunk;
> > + }
> > + else
> > + shheader = dsa_get_address(area, shhashent->body);
> > +
> > + /*
> > + * We expose this shared entry now. You might think that the entry
> > + * can be removed by a concurrent backend, but since we are creating
> > + * an stats entry, the object actually exists and used in the upper
> > + * layer. Such an object cannot be dropped until the first vacuum
> > + * after the current transaction ends.
> > + */
> > + dshash_release_lock(pgStatSharedHash, shhashent);
>
> I don't think you can safely release the lock before you incremented the
> refcount? What if, once the lock is released, somebody looks up that
> entry, increments the refcount, and decrements it again? It'll see a
> refcount of 0 at the end and decide to free the memory. Then the code
> below will access already freed / reused memory, no?

Yep, it's not even particularly hard to hit:

S0: CREATE TABLE a_table();
S0: INSERT INTO a_table();
S0: disconnect
S1: set a breakpoint to just after the dshash_release_lock(), with an
if objid == a_table_oid
S1: SELECT pg_stat_get_live_tuples('a_table'::regclass);
(will break at above breakpoint, without having incremented the
refcount yet)
S2: DROP TABLE a_table;
S2: VACUUM pg_class;

At that point S2's call to pgstat_vacuum_stat() will find the shared
stats entry for a_table, delete the entry from the shared hash table,
see that the stats data has a zero refcount, and free it. Once S1 wakes
up it'll use already freed (and potentially since reused) memory.

Greetings,

Andres Freund