Re: Sloppiness around failure handling of parsePGArray in pg_dump

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