[Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test could free() NULL pointers.

[Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test could free() NULL pointers.

[Emanuele Giuseppe Esposito]

server->bus in _test_server_free() could be NULL, since TestServer *dest in
test_migrate() was not properly initialized like TestServer *s. Added
init_virtio_dev(dest) and uninit_virtio_dev(dest), so the fields are properly
set and when test_server_free(dest); is called, they can be correctly freed.

Same applies to s->pci.bus, since it is set depending on the architecture.

Problem came out once I modified  pci-pc.c and pci-pc.h, modifying QPCIBusPC
by adding another field before QPCIBus bus. Re-running the tests showed
vhost-user-test failing.

Signed-off-by: Emanuele Giuseppe Esposito <esposem@usi.ch>
---
 tests/libqos/pci-pc.c   | 2 ++
 tests/sdhci-test.c      | 4 +++-
 tests/vhost-user-test.c | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index a7803308b7..c126b08ff5 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -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);
+    }
     g_free(s->pci.dev);
     qtest_quit(global_qtest);
     g_free(s);
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 8ff2106d32..a8a02c45cd 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -684,6 +684,7 @@ static void test_migrate(void)
     g_free(cmd);
 
     init_virtio_dev(s, 1u << VIRTIO_NET_F_MAC);
+    init_virtio_dev(dest, 1u << VIRTIO_NET_F_MAC);
     wait_for_fds(s);
     size = get_log_size(s);
     g_assert_cmpint(size, ==, (2 * 1024 * 1024) / (VHOST_LOG_PAGE * 8));
@@ -739,6 +740,7 @@ static void test_migrate(void)
     read_guest_mem_server(dest);
 
     uninit_virtio_dev(s);
+    uninit_virtio_dev(dest);
 
     g_source_destroy(source);
     g_source_unref(source);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test could free() NULL pointers.

[Paolo Bonzini]

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.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2] qpci_free_pc: sdhci-test and vhost-user-test could free() NULL pointers.

[Emanuele]

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