Re: [Qemu-devel] [PATCH v3 07/42] sdhci: refactor common sysbus/pci unrealize() into sdhci_unrealizefn()

[Philippe Mathieu-Daudé]

> On 01/03/2018 04:56 AM, Fam Zheng wrote:
>> On Fri, 12/29 14:48, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>>  hw/sd/sdhci.c | 19 ++++++++++++++++---
>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index ad5853d527..06a1ec6f91 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -31,6 +31,7 @@
>>>  #include "qemu/bitops.h"
>>>  #include "hw/sd/sdhci.h"
>>>  #include "sdhci-internal.h"
>>> +#include "qapi/error.h"
>>>  #include "qemu/log.h"
>>>
>>>  /* host controller debug messages */
>>> @@ -1203,15 +1204,17 @@ static void sdhci_realizefn(SDHCIState *s, Error 
>>> **errp)
>>>                            SDHC_REGISTERS_MAP_SIZE);
>>>  }
>>>
>>> +static void sdhci_unrealizefn(SDHCIState *s, Error **errp)
>>> +{
>>> +    g_free(s->fifo_buffer);
>>
>> Set s->fifo_buffer to NULL to avoid double-free and/or use-after-free?
>> Especially since you call this from both the ->exit and the ->unrealize
>> callbacks.
>
> Yes, I agree this would be safer. I'll also add a comment around.

The sysbus class sets the DeviceClass->unrealize(),
the pci class only sets PCIDeviceClass->exit(),
so in both cases sdhci_unrealizefn() is only called once.

Still, better to set this to NULL to protect any further use/refactor
of this code.

>>> +}
>>> +
>>>  static void sdhci_uninitfn(SDHCIState *s)
>>>  {
>>>      timer_del(s->insert_timer);
>>>      timer_free(s->insert_timer);
>>>      timer_del(s->transfer_timer);
>>>      timer_free(s->transfer_timer);
>>> -
>>> -    g_free(s->fifo_buffer);
>>> -    s->fifo_buffer = NULL;
>>>  }
>>>
>>>  static bool sdhci_pending_insert_vmstate_needed(void *opaque)
>>> @@ -1312,6 +1315,8 @@ static void sdhci_pci_realize(PCIDevice *dev, Error 
>>> **errp)
>>>  static void sdhci_pci_exit(PCIDevice *dev)
>>>  {
>>>      SDHCIState *s = PCI_SDHCI(dev);
>>> +
>>> +    sdhci_unrealizefn(s, &error_abort);
>>>      sdhci_uninitfn(s);
>>>  }
>>>
>>> @@ -1365,11 +1370,19 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
>>> Error ** errp)
>>>      sysbus_init_mmio(sbd, &s->iomem);
>>>  }
>>>
>>> +static void sdhci_sysbus_unrealize(DeviceState *dev, Error **errp)
>>> +{
>>> +    SDHCIState *s = SYSBUS_SDHCI(dev);
>>> +
>>> +    sdhci_unrealizefn(s, errp);
>>> +}
>>> +
>>>  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>
>>>      dc->realize = sdhci_sysbus_realize;
>>> +    dc->unrealize = sdhci_sysbus_unrealize;
>>>
>>>      sdhci_class_init(klass, data);
>>>  }
>>> --
>>> 2.15.1
>>>
>>>
>>
>> Fam
>>