Thread: Sloppiness around failure handling of parsePGArray in pg_dump
Hi all, Following the report of Coverity that led to 3636efa, I have reviewed the existing callers of parsePGArray() in pg_dump and some of its error handling is a bit sloppy. It could theoretically be possible to reach an OOM in parsePGArray() with a dump able to finish. This is very unlikely going to matter in practice as an OOM when parsing an array is most likely going to trigger a fatal failure in one of the follow-up allocations, but if the dump is able to go through we could finish with a valid dump that lacks some information: - Statistics for indexes. - Run-time configuration of functions. - Configuration of extensions. - Publication list for a subscription. I would like to propose the attached to tighten the error handling in the area, generating a fatal error if an array cannot be parsed. I did not see the point of changing the assumptions we use for the parsing of function args or such when it comes to pre-8.4 dumps. This issue is unlikely going to matter in practice, so I don't propose a backpatch. Thoughts? -- Michael
Attachment
> On 11 Nov 2020, at 07:13, Michael Paquier <michael@paquier.xyz> wrote:
> I would like to propose the attached to tighten the error handling in
> the area, generating a fatal error if an array cannot be parsed.
I agree that we should fix this even if it will have quite limited impact in
production settings. Patch LGTM, +1.
> I did not see the point of changing the assumptions we use for the parsing of
> function args or such when it comes to pre-8.4 dumps.
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;
> This issue is unlikely going to matter in practice, so I don't propose a
> backpatch.
Agreed, unless it's easier for dealing with backpatching other things, that
would be the only reason I reckon.
cheers ./daniel
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
Attachment
> On 19 Nov 2020, at 02:37, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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
Ah, yes, I read that wrong. It's correct as it is.
cheers ./daniel