Re: pg_stat_get_activity(): integer overflow due to (int) * (int) for MemoryContextAllocHuge()

On Fri, Sep 29, 2023 at 4:00 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Sep 28, 2023 at 11:01:14AM +0200, Jakub Wartak wrote:
> > v3 attached. I had a problem coming out with a better error message,
> > so suggestions are welcome. The cast still needs to be present as per
> > above suggestion as 3GB is still valid buf size and still was causing
> > integer overflow. We just throw an error on >= 4GB with v3.
>
> +/* Safety net to prevent requesting huge memory by each query to pg_stat_activity */
> +#define PGSTAT_MAX_ACTIVITY_BUF_SIZE 4 * 1024 * 1024 * 1024L
>
> - size = add_size(size,
> - mul_size(pgstat_track_activity_query_size, NumBackendStatSlots));
> + pgstat_track_size = mul_size(pgstat_track_activity_query_size,
> + NumBackendStatSlots);
> + if(pgstat_track_size >= PGSTAT_MAX_ACTIVITY_BUF_SIZE)
> + elog(FATAL, "too big Backend Activity Buffer allocation of %zu bytes", pgstat_track_size);
> + size = add_size(size, pgstat_track_size);
>
> That should be enough to put in a hardcoded 4GB safety limit, while
> mul_size() detects it at a higher range. Note, however, that elog()
> is only used for internal errors that users should never face, but
> this one can come from a misconfiguration. This would be better as an
> ereport(), with ERRCODE_CONFIG_FILE_ERROR as errcode, I guess.
>
> "Backend Activity Buffer" is the name of the internal struct. Sure,
> it shows up on the system views for shmem, but I wouldn't use this
> term in a user-facing error message. Perhaps something like "size
> requested for backend status is out of range" would be cleaner. Other
> ideas are welcome.

Hi Michael,

I've attached v4 that covers your suggestions.

-J.