[dm-devel] [PATCH 1/1] multipath: fix memory leak and segfault in reconfigure

[Christophe Varoqui]

Hi Germano,

no reason at all ... it just missed my attention, twice. Sorry.

This patch is now merged.
Thanks,
Christophe Varoqui
www.opensvc.com

On Mon, Apr 18, 2016 at 12:42 PM, Germano Percossi <
germano.percossi at citrix.com> wrote:

> Hi Christophe,
>
> Is there a specific reason this silly patch is not getting
> through?
>
> It seems a no-brainer to me but probably I am missing something.
>
> Thanks,
> Germano
>
> On 02/11/2016 07:30 PM, Germano Percossi wrote:
>
>> Within the reconfigure function, the global pointer conf is
>> stored in a local variable and then assigned NULL.
>> If load_config should fail, for any reason, we end up with
>> a memory leak, as soon as we leave the function, and with
>> the global pointer conf set to NULL, leading to a segfault
>> as soon as it is dereferenced.
>>
>> I tested it by calling a reconfigure and making the first
>> allocation in load_config fail but any failure in load_config
>> would do.
>>
>>> From a user perspective the CLI reports "fail".
>>>
>>
>> If something like this should happen there are at least 2 possible
>> scenarios:
>>
>> 1) If a second immediate reconfigure succeeds, the conf now is fine but
>>     the leak stays
>> 2) If the previous point does not happen, any command trying to access
>>     "conf" would fail. On my test box a "show conf" segfaulted.
>>
>> The fix is simple but in case of failure at least the previous
>> conf is kept in memory without leaks or segfaluts
>>
>> Signed-off-by: Germano Percossi <germano.percossi at citrix.com>
>> ---
>>   multipathd/main.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/multipathd/main.c b/multipathd/main.c
>> index 04f6d02..f83c849 100644
>> --- a/multipathd/main.c
>> +++ b/multipathd/main.c
>> @@ -1551,6 +1551,8 @@ reconfigure (struct vectors * vecs)
>>                 configure(vecs, 1);
>>                 free_config(old);
>>                 retval = 0;
>> +       } else {
>> +               conf = old;
>>         }
>>
>>         running_state = DAEMON_RUNNING;
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20160418/77886e0e/attachment.htm>