[2/8] zeroconf-discover: fix memory issues

Submitted by Yclept Nemo on July 17, 2018, 12:25 a.m.

Details

Message ID 20180717002544.5183-3-orbisvicis@gmail.com
State New
Series "*** Overview ***"
Headers show

Commit Message

Yclept Nemo July 17, 2018, 12:25 a.m.
Fix memory leak and prevent posible double-free.
---
 src/modules/module-zeroconf-discover.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/module-zeroconf-discover.c b/src/modules/module-zeroconf-discover.c
index bc61b403..2c49d13a 100644
--- a/src/modules/module-zeroconf-discover.c
+++ b/src/modules/module-zeroconf-discover.c
@@ -379,10 +379,17 @@  static void client_callback(AvahiClient *c, AvahiClientState state, void *userda
 
                 pa_log_debug("Avahi daemon disconnected.");
 
-                if (!(u->client = avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error))) {
+                /* Frees all associated resources, i.e. browsers, resolvers,
+                 * and groups. */
+                avahi_client_free(c);
+                u->client = u->sink_browser = u->source_browser = NULL;
+
+                if (!avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error)) {
                     pa_log("avahi_client_new() failed: %s", avahi_strerror(error));
                     pa_module_unload_request(u->module, true);
                 }
+
+                break;
             }
 
             /* Fall through */
@@ -442,14 +449,21 @@  int pa__init(pa_module*m) {
     m->userdata = u = pa_xnew(struct userdata, 1);
     u->core = m->core;
     u->module = m;
-    u->sink_browser = u->source_browser = NULL;
+    u->client = u->sink_browser = u->source_browser = NULL;
     u->protocol = protocol;
 
     u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
 
     u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
 
-    if (!(u->client = avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error))) {
+    /* The client callback is run for the first time within 'avahi_client_new',
+     * and on AVAHI_CLIENT_FAILURE may free the old client and create a new
+     * client assigned to 'userdata.client'. If so 'avahi_client_new' will
+     * return a pointer to already-freed data. When 'avahi_client_new' fails it
+     * returns NULL and does not run the callback; 'userdata.client' remains
+     * NULL (see above). Otherwise the callback is run, ensuring that
+     * 'userdata.client' is appropriately set. */
+    if (!avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error)) {
         pa_log("pa_avahi_client_new() failed: %s", avahi_strerror(error));
         goto fail;
     }

Comments

Tanu Kaskinen Aug. 11, 2018, 1:23 p.m.
On Mon, 2018-07-16 at 20:25 -0400, Yclept Nemo wrote:
> Fix memory leak and prevent posible double-free.
> ---
>  src/modules/module-zeroconf-discover.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/modules/module-zeroconf-discover.c b/src/modules/module-zeroconf-discover.c
> index bc61b403..2c49d13a 100644
> --- a/src/modules/module-zeroconf-discover.c
> +++ b/src/modules/module-zeroconf-discover.c
> @@ -379,10 +379,17 @@ static void client_callback(AvahiClient *c, AvahiClientState state, void *userda
>  
>                  pa_log_debug("Avahi daemon disconnected.");
>  
> -                if (!(u->client = avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error))) {
> +                /* Frees all associated resources, i.e. browsers, resolvers,
> +                 * and groups. */
> +                avahi_client_free(c);
> +                u->client = u->sink_browser = u->source_browser = NULL;

GCC doesn't like this:

modules/module-zeroconf-discover.c: In function ‘client_callback’:
modules/module-zeroconf-discover.c:385:27: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
                 u->client = u->sink_browser = u->source_browser = NULL;
                           ^

> +
> +                if (!avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error)) {

It would be good to have a comment, something like this:

/* We don't set u->client here, the reason is explained in pa__init(). */

>                      pa_log("avahi_client_new() failed: %s", avahi_strerror(error));
>                      pa_module_unload_request(u->module, true);
>                  }
> +
> +                break;
>              }
>  
>              /* Fall through */
> @@ -442,14 +449,21 @@ int pa__init(pa_module*m) {
>      m->userdata = u = pa_xnew(struct userdata, 1);
>      u->core = m->core;
>      u->module = m;
> -    u->sink_browser = u->source_browser = NULL;
> +    u->client = u->sink_browser = u->source_browser = NULL;

modules/module-zeroconf-discover.c: In function ‘module_zeroconf_discover_LTX_pa__init’:
modules/module-zeroconf-discover.c:452:15: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
     u->client = u->sink_browser = u->source_browser = NULL;
               ^

>      u->protocol = protocol;
>  
>      u->tunnels = pa_hashmap_new(tunnel_hash, tunnel_compare);
>  
>      u->avahi_poll = pa_avahi_poll_new(m->core->mainloop);
>  
> -    if (!(u->client = avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error))) {
> +    /* The client callback is run for the first time within 'avahi_client_new',
> +     * and on AVAHI_CLIENT_FAILURE may free the old client and create a new
> +     * client assigned to 'userdata.client'. If so 'avahi_client_new' will
> +     * return a pointer to already-freed data. When 'avahi_client_new' fails it
> +     * returns NULL and does not run the callback; 'userdata.client' remains
> +     * NULL (see above). Otherwise the callback is run, ensuring that
> +     * 'userdata.client' is appropriately set. */
> +    if (!avahi_client_new(u->avahi_poll, AVAHI_CLIENT_NO_FAIL, client_callback, u, &error)) {
>          pa_log("pa_avahi_client_new() failed: %s", avahi_strerror(error));
>          goto fail;
>      }