Hi,
On 2022-09-07 11:08:27 -0400, Tom Lane wrote:
> > I'll need to think about how best to fix this. In the meantime, I
> > think the other 32-bit animals are probably not going to like this
> > either :-(
>
> Yeah. The basic problem here is that we've greatly reduced the amount
> of redundancy in chunk headers.
Even with the prior amount of redundancy it's quite scary. It's one thing if
the only consequence is a bit of added overhead - but if we *don't* copy the
tuple due to a false positive we're in trouble. Afaict the user would have
some control over the memory contents and so an attacker could work on
triggering this issue. MemoryContextContains() may be ok for an assertion or
such, but relying on it for correctness seems a bad idea.
I wonder if we can get away from needing these checks, without unnecessarily
copying datums every time:
If there is no finalfunc, we know that the tuple ought to be in curaggcontext
or such, and we need to copy.
If there is a finalfunc, they're typically going to return data from within
the current memory context, but could legitimately also return part of the
data from curaggcontext. Perhaps we could forbid that? Our docs already say
the following for serialization funcs:
The result of the deserialization function should simply be allocated in the
current memory context, as unlike the combine function's result, it is not
long-lived.
Perhaps we could institute a similar rule for finalfuncs? The argument against
that is that we can use arbitrary functions as finalfuncs currently. Perhaps
we could treat taking an internal argument as opting into the requirement and
default to copying otherwise?
Greetings,
Andres Freund
