[Qemu-devel] [PATCH] slirp: fix segv when init failed

[Qemu-devel] [PATCH] slirp: fix segv when init failed

[Marc-André Lureau]

Since commit f6c2e66ae8c8a, slirp uses an exit notifier to call
slirp_smb_cleanup. However, if init() failed, the notifier isn't added,
and removing it will fail:

==18447== Invalid write of size 8
==18447==    at 0x7EF2B5: notifier_remove (notify.c:32)
==18447==    by 0x48E80C: qemu_remove_exit_notifier (vl.c:2661)
==18447==    by 0x6A2187: net_slirp_cleanup (slirp.c:134)
==18447==    by 0x69419D: qemu_cleanup_net_client (net.c:338)
==18447==    by 0x69445B: qemu_del_net_client (net.c:401)
==18447==    by 0x6A2B81: net_slirp_init (slirp.c:366)
==18447==    by 0x6A4241: net_init_slirp (slirp.c:865)
==18447==    by 0x695C6D: net_client_init1 (net.c:1051)
==18447==    by 0x695F6E: net_client_init (net.c:1108)
==18447==    by 0x696DBA: net_init_netdev (net.c:1498)
==18447==    by 0x7F1F99: qemu_opts_foreach (qemu-option.c:1116)
==18447==    by 0x696E60: net_init_clients (net.c:1516)
==18447==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/slirp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index facc30e..b60893f 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -131,7 +131,9 @@ static void net_slirp_cleanup(NetClientState *nc)
     SlirpState *s = DO_UPCAST(SlirpState, nc, nc);
 
     slirp_cleanup(s->slirp);
-    qemu_remove_exit_notifier(&s->exit_notifier);
+    if (s->exit_notifier.notify) {
+        qemu_remove_exit_notifier(&s->exit_notifier);
+    }
     slirp_smb_cleanup(s);
     QTAILQ_REMOVE(&slirp_stacks, s, entry);
 }
-- 
2.9.0

Re: [Qemu-devel] [PATCH] slirp: fix segv when init failed

[Jason Wang]

On 2016年08月18日 21:44, Marc-André Lureau wrote:
> Since commit f6c2e66ae8c8a, slirp uses an exit notifier to call
> slirp_smb_cleanup. However, if init() failed, the notifier isn't added,
> and removing it will fail:
>
> ==18447== Invalid write of size 8
> ==18447==    at 0x7EF2B5: notifier_remove (notify.c:32)
> ==18447==    by 0x48E80C: qemu_remove_exit_notifier (vl.c:2661)
> ==18447==    by 0x6A2187: net_slirp_cleanup (slirp.c:134)
> ==18447==    by 0x69419D: qemu_cleanup_net_client (net.c:338)
> ==18447==    by 0x69445B: qemu_del_net_client (net.c:401)
> ==18447==    by 0x6A2B81: net_slirp_init (slirp.c:366)
> ==18447==    by 0x6A4241: net_init_slirp (slirp.c:865)
> ==18447==    by 0x695C6D: net_client_init1 (net.c:1051)
> ==18447==    by 0x695F6E: net_client_init (net.c:1108)
> ==18447==    by 0x696DBA: net_init_netdev (net.c:1498)
> ==18447==    by 0x7F1F99: qemu_opts_foreach (qemu-option.c:1116)
> ==18447==    by 0x696E60: net_init_clients (net.c:1516)
> ==18447==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   net/slirp.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index facc30e..b60893f 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -131,7 +131,9 @@ static void net_slirp_cleanup(NetClientState *nc)
>       SlirpState *s = DO_UPCAST(SlirpState, nc, nc);
>   
>       slirp_cleanup(s->slirp);
> -    qemu_remove_exit_notifier(&s->exit_notifier);
> +    if (s->exit_notifier.notify) {
> +        qemu_remove_exit_notifier(&s->exit_notifier);
> +    }
>       slirp_smb_cleanup(s);
>       QTAILQ_REMOVE(&slirp_stacks, s, entry);
>   }

Applied to -net for 2.7.

Thanks

Re: [Qemu-devel] [PATCH] slirp: fix segv when init failed

[Samuel Thibault]

Marc-André Lureau, on Thu 18 Aug 2016 17:44:05 +0400, wrote:
> Since commit f6c2e66ae8c8a, slirp uses an exit notifier to call
> slirp_smb_cleanup. However, if init() failed, the notifier isn't added,
> and removing it will fail:
> 
> ==18447== Invalid write of size 8
> ==18447==    at 0x7EF2B5: notifier_remove (notify.c:32)
> ==18447==    by 0x48E80C: qemu_remove_exit_notifier (vl.c:2661)
> ==18447==    by 0x6A2187: net_slirp_cleanup (slirp.c:134)
> ==18447==    by 0x69419D: qemu_cleanup_net_client (net.c:338)
> ==18447==    by 0x69445B: qemu_del_net_client (net.c:401)
> ==18447==    by 0x6A2B81: net_slirp_init (slirp.c:366)
> ==18447==    by 0x6A4241: net_init_slirp (slirp.c:865)
> ==18447==    by 0x695C6D: net_client_init1 (net.c:1051)
> ==18447==    by 0x695F6E: net_client_init (net.c:1108)
> ==18447==    by 0x696DBA: net_init_netdev (net.c:1498)
> ==18447==    by 0x7F1F99: qemu_opts_foreach (qemu-option.c:1116)
> ==18447==    by 0x696E60: net_init_clients (net.c:1516)
> ==18447==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  net/slirp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index facc30e..b60893f 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -131,7 +131,9 @@ static void net_slirp_cleanup(NetClientState *nc)
>      SlirpState *s = DO_UPCAST(SlirpState, nc, nc);
>  
>      slirp_cleanup(s->slirp);
> -    qemu_remove_exit_notifier(&s->exit_notifier);
> +    if (s->exit_notifier.notify) {
> +        qemu_remove_exit_notifier(&s->exit_notifier);
> +    }
>      slirp_smb_cleanup(s);
>      QTAILQ_REMOVE(&slirp_stacks, s, entry);
>  }
> -- 
> 2.9.0
> 
> 

-- 
Samuel
<N> bon comment on fait de l'investigation pour savoir qui est le vilain ?
<s> on débranche le routeur et on regarde qui s'affole
 -+- #ens-mim administre -+-