> 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é <f4bug@amsat.org>
>>> ---
>>> 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
>>