On Wed, Nov 18, 2020 at 10:19:40AM +0100, Daniel Gustafsson wrote: > I agree that we should fix this even if it will have quite limited impact in > production settings. Patch LGTM, +1.
Thanks. I have reviewed that again this morning and applied it. > Another thing caught my eye here (while not the fault of this patch), we > ensure > to clean up array leftover in case of parsePGArray failure, but we don't clean > up the potential allocations from the previous calls. Something like the > below > seems more consistent. > > @@ -12105,6 +12099,8 @@ dumpFunc(Archive *fout, FuncInfo *finfo) > nitems != nallargs) > { > pg_log_warning("could not parse proargmodes array"); > + if (allargtypes) > + free(allargtypes); > if (argmodes) > free(argmodes); > argmodes = NULL; > @@ -12119,6 +12115,10 @@ dumpFunc(Archive *fout, FuncInfo *finfo) > nitems != nallargs) > { > pg_log_warning("could not parse proargnames array"); > + if (allargtypes) > + free(allargtypes); > + if (argmodes) > + free(argmodes); > if (argnames) > free(argnames); > argnames = NULL; If you do that, I think that's not completely correct either. format_function_arguments_old() has some logic to allow the process to go through for pre-8.4 dumps as long as allargtypes includes correct data even if argmodes and/or argnames parsing has failed, so you would cause a crash as long as nallargs is set if you free allargtypes like that. You could allow those free calls if resetting nallargs to 0 if the parsing of argmodes or argnames has failed, but that would make the logic less flexible. -- Michael
signature.asc
Description: PGP signature