Re: Memory leak in spapr_machine_init()?

[Markus Armbruster <armbru@redhat.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Thu, Jun 18, 2020 at 08:55:53AM +0200, Markus Armbruster wrote:
>> Either I'm confused (quite possible), or kvmppc_check_papr_resize_hpt()
>> can leak an Error object on failure.  Please walk through the code with
>> me:
>> 
>>         kvmppc_check_papr_resize_hpt(&resize_hpt_err);
>> 
>> This sets @resize_hpt_err on failure.
>> 
>>         if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
>>             /*
>>              * If the user explicitly requested a mode we should either
>>              * supply it, or fail completely (which we do below).  But if
>>              * it's not set explicitly, we reset our mode to something
>>              * that works
>>              */
>>             if (resize_hpt_err) {
>>                 spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
>>                 error_free(resize_hpt_err);
>>                 resize_hpt_err = NULL;
>> 
>> Case 1: failure and SPAPR_RESIZE_HPT_DEFAULT; we free @resize_hpt_err.
>> Good.
>> 
>>             } else {
>>                 spapr->resize_hpt = smc->resize_hpt_default;
>>             }
>>         }
>> 
>>         assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
>> 
>>         if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && resize_hpt_err) {
>>             /*
>>              * User requested HPT resize, but this host can't supply it.  Bail out
>>              */
>>             error_report_err(resize_hpt_err);
>>             exit(1);
>> 
>> Case 2: failure and not SPAPR_RESIZE_HPT_DISABLED; fatal.  Good.
>> 
>>         }
>> 
>> What about case 3: failure and SPAPR_RESIZE_HPT_DISABLED?
>> 
>> Good if we get here via case 1 (we freed @resize_hpt_err).
>> 
>> Else, ???
>
> I think you're right, and we leak it in this case - I think I forgot
> that in the DISABLED case we still (unnecessarily) ask the kernel if
> it can do it.
>
> Of course, it will only happen once per run, so it's not like it's a
> particularly noticeable leak.

Understood.  I'll post a patch.  Thanks!