On Mon, Aug 30, 2021 at 01:01:16PM +0530, Bharath Rupireddy wrote:
> Shound't we fix it in master branch to keep the code in sync with
> other places where we usually follow that kind of type-casting? IMO,
> we should just make that change, because it isn't a major change or we
> aren't going to back patch it.
One thing is that this creates conflicts with back-branches, and
that's always annoying. I'd be fine with changing new code for that,
though.
>> Here you want to free sysidentifier only if it has been set, and
>> RunIdentifySystem() may fail before doing that, so you should assign
>> NULL to sysidentifier when it is declared.
>
> Isn't the pg_free going to take care of sysidentifier being null?
> if (ptr != NULL)
> free(ptr);
It would, but you don't initialize the variable to begin with, so you
may finish with freeing a pointer that points to nothing, and crash
any code using ReceiveXlogStream() while some code paths should be
able to handle retries. I guess that compilers would not complain
here because they cannot understand that RunIdentifySystem() may not
set up the variable before this function returns. I have fixed this
initialization, and committed the patch. Note that there is only one
other code path using RunIdentifySystem() with the system ID as of
pg_basebackup.c, but this one would just exit() if we fail to run the
command, so we don't need to care about freeing the system ID there.
--
Michael
