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
signature.asc
Description: PGP signature
