On Fri, 22 Nov 2024 at 22:13, Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2024-07-09 13:12:59 -0500, Nathan Bossart wrote: > > I've committed 0001. > > I justed ended up looking at this code for some boring reason. One thing that > has me worried a bit is that pg_signal_backend() now does > pgstat_get_beentry_by_proc_number(), triggering a pgstat_read_current_status() > further down. > > pgstat_read_current_status() can be fairly expensive, both in CPU and in > memory. It copies each proc's activity strings, which each can be 1kB by > default! > > The "saving grace" is that most of the time the pid will be sourced from > pg_stat_activity, which already will will have done > pgstat_read_current_status(). But I don't think that's always the case. > > > Another concern is that pgstat_read_current_status() won't actually refresh > the information if already cached, which might cause us to operate on outdated > information. A malicious user could start a transaction, cause > pgstat_read_current_status() to be called, and wait for the PID to be reused > for some interesting process, and then signal, which the outdated information > from the prior pgstat_read_current_status() would allow. > > The threat of this isn't huge, there's some fundamental raciness around > pg_signal_backend() but this does open the window a lot wider. > > > And all of this just because we need the target proc's BackendType, a single 4 > byte value. > > I think it'd be better to introduce something that fetches a live > BackendType. We have such functionality already, see > pgstat_get_backend_current_activity(). > > Or we could have a copy of the backend type in PGPROC. > > Greetings, > > Andres Freund
Hi, thanks for taking care of this. I agree with this analysis, and it appears that the worries are legitimate. For the fix, I believe that copy-pasting `pgstat_get_backend_current_activity` to get the backend type should solve the issue. Not sure if this is the correct way of doing this. Enlarging PGPROC somehow feels worse. -- Best regards, Kirill Reshke
