[PATCH] ch: set driver to NULL after freeing it

Daniel P. Berrangé posted 1 patch 2 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210604150104.155725-1-berrange@redhat.com
src/ch/ch_driver.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] ch: set driver to NULL after freeing it
Posted by Daniel P. Berrangé 2 years, 9 months ago
If the chStateInitialize method fails, we call chStateCleanup
which free's all global state. It fails to set the global
'ch_driver' to NULL, however, so a later attempt to open the
cloud hypervisor driver will succeed and then crash attempting
to access freed memory.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/ch/ch_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 8c458a20bd..1ee33817f9 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -827,6 +827,7 @@ static int chStateCleanup(void)
     virObjectUnref(ch_driver->config);
     virMutexDestroy(&ch_driver->lock);
     g_free(ch_driver);
+    ch_driver = NULL;
 
     return 0;
 }
-- 
2.31.1

Re: [PATCH] ch: set driver to NULL after freeing it
Posted by Michal Prívozník 2 years, 9 months ago
On 6/4/21 5:01 PM, Daniel P. Berrangé wrote:
> If the chStateInitialize method fails, we call chStateCleanup
> which free's all global state. It fails to set the global
> 'ch_driver' to NULL, however, so a later attempt to open the
> cloud hypervisor driver will succeed and then crash attempting
> to access freed memory.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/ch/ch_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 8c458a20bd..1ee33817f9 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -827,6 +827,7 @@ static int chStateCleanup(void)
>      virObjectUnref(ch_driver->config);
>      virMutexDestroy(&ch_driver->lock);
>      g_free(ch_driver);
> +    ch_driver = NULL;
>  
>      return 0;
>  }
> 

Oh, I missed this completely before sending my patch:

https://listman.redhat.com/archives/libvir-list/2021-June/msg00158.html

How about g_clear_pointer() instead? Regardless:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal