Re: [sqlsmith] Crash reading pg_stat_activity

Thomas Munro writes:

> On Wed, Dec 28, 2016 at 11:38 AM, Andreas Seltenreich
> <seltenreich(at)gmx(dot)de> wrote:
>> Thomas Munro writes:
>>
>>> On Wed, Dec 28, 2016 at 10:40 AM, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>>> On Wed, Dec 28, 2016 at 10:01 AM, Andreas Seltenreich <seltenreich(at)gmx(dot)de> wrote:
>>>>> testing master as of fe591f8bf6 produced a crash reading
>>>>> pg_stat_activity (backtrace below). Digging around with with gdb
>>>>> revealed that pgstat_get_wait_event() returned an invalid pointer for a
>>>>> classId PG_WAIT_LWLOCK.
>>>>>
>>>>> I think the culprit is dsa.c passing a pointer to memory that goes away
>>>>> on dsa_free() as a name to LWLockRegisterTranche.
>> [..]
>>>> Maybe we should replace it with another value when the DSA area is
>>>> detached, using a constant string. Something like
>>
>> I'm wondering: Is it safe to pass a pointer into a DSA at all? If I
>> understand the comments correctly, they are not necessarily mapped (at
>> the same address) in an unrelated backend looking into pg_stat_activity,
>> and in this case a dsa_free() is not actually needed to trigger a crash.
>
> It is safe, as long as the segment remains mapped. Each backend that
> attaches calls LWLockRegisterTranche giving it the address of the name
> in its virtual address space.

Hmok, I was under the impression only backends participating in the IPC
call the attach function, not necessarily the ones that could possible
want to resolve the wait_event_info they found in the procArray via
pgstat_get_wait_event().

>> Maybe instead of copying the name, just put the passed pointer itself
>> into the area? Extensions using LWLockNewTrancheId need to use
>> shared_preload_libraries anyway, so static strings would be mapped in
>> all backends.
>
> Yeah that would be another way. I had this idea that only the process
> that creates a DSA area should name it, and then processes attaching
> would see the existing tranche ID and name, so could use a narrower
> interface. We could instead do as you say and make processes that
> attach provide a pointer to the name too, and make it the caller's
> problem to ensure that the pointers remain valid long enough; or go
> one step further and make them register/unregister it themselves.

Hmm, turning the member of the control struct
char lwlock_tranche_name[DSA_MAXLEN];
into
const char *lwlock_tranche_name;
and initializing it with the passed static const char * instead of
copying wouldn't require a change of the interface, would it?

But I really feel like I need to study the code a bit more before
commenting further…

regards,
Andreas