qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test


From: Emanuele
Subject: Re: [Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test could free() NULL pointers.
Date: Mon, 2 Jul 2018 20:04:42 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0



On 07/02/2018 04:35 PM, Paolo Bonzini wrote:
On 02/07/2018 16:05, Emanuele Giuseppe Esposito wrote:
@@ -152,6 +152,8 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
*alloc)
void qpci_free_pc(QPCIBus *bus)
  {
+    g_assert(bus);
+
      QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
g_free(s);
diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 1d825eb010..9b486b93bf 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -209,7 +209,9 @@ static QSDHCI *machine_start(const struct sdhci_t *test)
static void machine_stop(QSDHCI *s)
  {
-    qpci_free_pc(s->pci.bus);
+    if (s->pci.bus) {
+        qpci_free_pc(s->pci.bus);
+    }
Sorry for chiming in just now.

In general, freeing a NULL pointer is a fine thing to do in C.  In your
code you do

     QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
     qpci_set_pc(ret, qts, alloc);
     return &ret->bus;

But now &ret->bus can be inside the pointer.  qpci_free_pc must
therefore check for NULL before doing the container_of.

It is debatable whether this change should go in QEMU before your code,
or together with it.  There are good arguments for both sides:

- the container_of is assuming that QPCIBus is the first field of the
struct, but that's a strange assumption: container_of usually is used to
go from an interior pointer to an outside struct, and passing NULL to it
is usually wrong

- but, the struct _does_ have QPCIBus as the first field, so we can
assume that if bus == NULL, s will be NULL too.  And g_free(NULL) is okay.

I suggest that you add the "if (!bus) { return; }" in your code, in the
same patch that adds the field before QPCIBusPC.
I see what you mean, I agree this should go in my main patch.

Thank you,

Emanuele

Thanks,

Paolo




reply via email to

[Prev in Thread] Current Thread [Next in Thread]