From: Jing Wu <wujing42@huawei.com>

Some functions like doCoreDump call virFileWrapperFdNew to execute async
cmds, if a step after virFileWrapperFdNew failed, the func may skip
virFileWrapperFdClose and jump to cleanup label to call
virFileWrapperFdFree directly. If the child process of the cmd is running
and asyncioThread is polling, cmd->errbuf have been alloced at least one
byte but not yet operate (*cmd->errbuf)[errlen] = '\0', access of
wfd->err_msg in virFileWrapperFdFree at this time will cause risk of
heap-buffer-overflow.
So, we need to put VIR_WARN(wfd->err_msg) after VIR_FREE(wfd->err_msg).
Besides, since virCommandFree has included virCommandAbort, there is no
need to call virCommandAbort extraly.

Signed-off-by: Jing Wu <wujing42@huawei.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
 src/util/virfile.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index f6f9e4c..d488158 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -347,13 +347,12 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
     if (!wfd)
         return;
 
+    virCommandFree(wfd->cmd);
+
     if (wfd->err_msg && *wfd->err_msg)
         VIR_WARN("iohelper reports: %s", wfd->err_msg);
 
-    virCommandAbort(wfd->cmd);
-
     VIR_FREE(wfd->err_msg);
-    virCommandFree(wfd->cmd);
     VIR_FREE(wfd);
 }
 
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Feb 19, 2019 at 03:40:31PM +0800, Jay Zhou wrote:
>From: Jing Wu <wujing42@huawei.com>
>
>Some functions like doCoreDump call virFileWrapperFdNew to execute async
>cmds, if a step after virFileWrapperFdNew failed, the func may skip
>virFileWrapperFdClose and jump to cleanup label to call
>virFileWrapperFdFree directly.

Wouldn't the proper fix be to always call virFileWrapperFdClose?
See this series by Andrea:
https://www.redhat.com/archives/libvir-list/2019-February/msg01155.html

>If the child process of the cmd is running
>and asyncioThread is polling, cmd->errbuf have been alloced at least one
>byte but not yet operate (*cmd->errbuf)[errlen] = '\0', access of
>wfd->err_msg in virFileWrapperFdFree at this time will cause risk of
>heap-buffer-overflow.

>So, we need to put VIR_WARN(wfd->err_msg) after VIR_FREE(wfd->err_msg).

This sentence does not make sense - why would we access the error after
freeing it?

Jano

>Besides, since virCommandFree has included virCommandAbort, there is no
>need to call virCommandAbort extraly.
>
>Signed-off-by: Jing Wu <wujing42@huawei.com>
>Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
>---
> src/util/virfile.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list