Re: Avoid memory leaks during base backups

On Thu, Oct 20, 2022 at 9:48 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Oct 20, 2022 at 6:47 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > I tried implementing this, please see the attached v7 patch.
>
> I haven't checked this in detail but it looks much more reasonable in
> terms of code footprint. However, we should, I think, set backup_state
> = NULL and tablespace_map = NULL before deleting the memory context.
> As you have it, I believe that if backup_state = (BackupState *)
> palloc0(sizeof(BackupState)) fails -- say due to running out of memory
> -- then those variables could end up pointing to garbage because the
> context had already been reset before initializing them. I don't know
> whether it's possible for that to cause any concrete harm, but nulling
> out the pointers seems like cheap insurance.

I think elsewhere in the code we reset dangling pointers either ways -
before or after deleting/resetting memory context. But placing them
before would give us extra safety in case memory context
deletion/reset fails. Not sure what's the best way. However, I'm
nullifying the dangling pointers after deleting/resetting memory
context.
MemoryContextDelete(Conf->buildCxt);
MemoryContextDelete(PostmasterContext);
MemoryContextDelete(rulescxt);

Please see the attached v8 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com