Fix for opaque data double free

diff --git a/src/include/radiusd.h b/src/include/radiusd.h
index 9c41f96..b6db397 100644 (file)
--- a/src/include/radiusd.h
+++ b/src/include/radiusd.h
@@ -534,6 +534,7 @@ int         log_err (char *);
 #define MEM(x) if (!(x)) { ERROR("Out of memory"); exit(1); }
 void (*reset_signal(int signo, void (*func)(int)))(int);
 void           request_free(REQUEST **request);
+int                    request_opaque_free(REQUEST *request);
 int            rad_mkdir(char *directory, mode_t mode);
 int            rad_checkfilename(char const *filename);
 int            rad_file_exists(char const *filename);
@@ -545,7 +546,7 @@ REQUEST             *request_alloc_fake(REQUEST *oldreq);
 REQUEST                *request_alloc_coa(REQUEST *request);
 int            request_data_add(REQUEST *request,
                                 void *unique_ptr, int unique_int,
-                                void *opaque, void (*free_opaque)(void *));
+                                void *opaque, bool free_opaque);
 void           *request_data_get(REQUEST *request,
                                  void *unique_ptr, int unique_int);
 void           *request_data_reference(REQUEST *request,

diff --git a/src/main/command.c b/src/main/command.c
index 4017b3e..f47109f 100644 (file)
--- a/src/main/command.c
+++ b/src/main/command.c
@@ -1320,7 +1320,7 @@ static int command_inject_file(rad_listen_t *listener, int argc, char *argv[])
                return 0;
        }
 
-       fake = rad_malloc(sizeof(*fake));
+       fake = talloc(NULL, rad_listen_t);
        memcpy(fake, sock->inject_listener, sizeof(*fake));
 
        /*
@@ -1385,8 +1385,8 @@ static int command_inject_file(rad_listen_t *listener, int argc, char *argv[])
         *      Remember what the output file is, and remember to
         *      delete the fake listener when done.
         */
-       request_data_add(request, null_socket_send, 0, strdup(buffer), free);
-       request_data_add(request, null_socket_send, 1, fake, free);
+       request_data_add(request, null_socket_send, 0, talloc_strdup(NULL, buffer), true);
+       request_data_add(request, null_socket_send, 1, fake, true);
 
 #endif
 

diff --git a/src/main/modcall.c b/src/main/modcall.c
index 5efc72c..bc37bc9 100644 (file)
--- a/src/main/modcall.c
+++ b/src/main/modcall.c
@@ -386,15 +386,6 @@ typedef struct modcall_stack {
        modcallable *start[MODCALL_STACK_MAX];
 } modcall_stack;
 
-
-#ifdef WITH_UNLANG
-static void pairfree_wrapper(void *data)
-{
-       VALUE_PAIR **vp = (VALUE_PAIR **) data;
-       pairfree(vp);
-}
-#endif
-
 /**
  * @brief Call a module, iteratively, with a local stack, rather than
  *     recursively.  What did Paul Graham say about Lisp...?
@@ -573,9 +564,15 @@ int modcall(int component, modcallable *c, REQUEST *request)
                                        copy = paircopy(request, vp);
                                        copy_p = ©
 
+                                       /*
+                                        *      @fixme: The old code freed copy on request_data_add or
+                                        *      request_data_get.  There's no way to easily do that now.
+                                        *      The foreach code should be audited for possible memory leaks,
+                                        *      though it's not a huge priority as any leaked memory will
+                                        *      be freed on request free.
+                                        */
                                        request_data_add(request, radius_get_vp,
-                                                        depth, copy_p,
-                                                        pairfree_wrapper);
+                                                        depth, copy_p, false);
 
                                        myresult = modcall(component,
                                                           g->children,

diff --git a/src/main/util.c b/src/main/util.c
index 505aa70..dc3fda1 100644 (file)
--- a/src/main/util.c
+++ b/src/main/util.c
@@ -83,19 +83,9 @@ struct request_data_t {
        void            *unique_ptr;
        int             unique_int;
        void            *opaque;
-       void            (*free_opaque)(void *);
+       bool            free_opaque;
 };
 
-static int request_data_free_opaque(void *ctx)
-{
-       request_data_t *this;
-
-       this = talloc_get_type_abort(ctx, request_data_t);
-       this->free_opaque(this->opaque);
-
-       return 0;
-}
-
 /*
  *     Add opaque data (with a "free" function) to a REQUEST.
  *
@@ -105,7 +95,7 @@ static int request_data_free_opaque(void *ctx)
  */
 int request_data_add(REQUEST *request,
                     void *unique_ptr, int unique_int,
-                    void *opaque, void (*free_opaque)(void *))
+                    void *opaque, bool free_opaque)
 {
        request_data_t *this, **last, *next;
 
@@ -122,11 +112,14 @@ int request_data_add(REQUEST *request,
 
                        next = this->next;
 
-                       if (this->opaque && /* free it, if necessary */
-                           this->free_opaque) {
-                               this->free_opaque(this->opaque);
+                       /*
+                        *      If caller requires custom behaviour on free
+                        *      they must set a destructor.
+                        */
+                       if (this->opaque && this->free_opaque) {
+                               talloc_free(this->opaque);
                        }
-                       break;  /* replace the existing entry */
+                       break;                          /* replace the existing entry */
                }
        }
 
@@ -136,10 +129,8 @@ int request_data_add(REQUEST *request,
        this->unique_ptr = unique_ptr;
        this->unique_int = unique_int;
        this->opaque = opaque;
-
        if (free_opaque) {
                this->free_opaque = free_opaque;
-               talloc_set_destructor((void *) this, request_data_free_opaque);
        }
 
        *last = this;
@@ -168,9 +159,8 @@ void *request_data_get(REQUEST *request,
                         *      Remove the entry from the list, and free it.
                         */
                        *last = this->next;
-                       talloc_set_destructor((void *) this, NULL);
                        talloc_free(this);
-                       return ptr; /* don't free it, the caller does that */
+                       return ptr;             /* don't free it, the caller does that */
                }
        }
 
@@ -242,6 +232,16 @@ void request_free(REQUEST **request_ptr)
 }
 
 /*
+ *     Free a request.
+ */
+int request_opaque_free(REQUEST *request)
+{
+       request_free(&request);
+
+       return 0;
+}
+
+/*
  *     Check a filename for sanity.
  *
  *     Allow only uppercase/lowercase letters, numbers, and '-_/.'
@@ -1031,14 +1031,6 @@ int radius_request(REQUEST **context, request_refs_t name)
        return 0;
 }
 
-/** Free subcapture string
- *
- * @param ptr to free.
- */
-static void _rad_regcapture_free(void *ptr) {
-       talloc_free(ptr);
-}
-
 /** Adds subcapture values to request data
  *
  * Allows use of %{n} expansions.
@@ -1066,7 +1058,7 @@ void rad_regcapture(REQUEST *request, int compare, char const *value, regmatch_t
                if ((compare != 0) || (rxmatch[i].rm_so == -1)) {
                        p = request_data_get(request, request, REQUEST_DATA_REGEX | i);
                        if (p) {
-                               _rad_regcapture_free(p);
+                               talloc_free(p);
                        }
 
                        continue;
@@ -1085,7 +1077,7 @@ void rad_regcapture(REQUEST *request, int compare, char const *value, regmatch_t
                 *      for out of memory, which is
                 *      the only error we can get...
                 */
-               request_data_add(request, request, REQUEST_DATA_REGEX | i, p, _rad_regcapture_free);
+               request_data_add(request, request, REQUEST_DATA_REGEX | i, p, true);
        }
 }
 

diff --git a/src/modules/rlm_eap/mem.c b/src/modules/rlm_eap/mem.c
index fc33717..912964d 100644 (file)
--- a/src/modules/rlm_eap/mem.c
+++ b/src/modules/rlm_eap/mem.c
@@ -90,12 +90,11 @@ eap_handler_t *eap_handler_alloc(rlm_eap_t *inst)
        return handler;
 }
 
-void eap_opaque_free(eap_handler_t *handler)
+int eap_opaque_free(eap_handler_t *handler)
 {
-       if (!handler)
-               return;
-
        eap_handler_free(handler->inst_holder, handler);
+
+       return 0;
 }
 
 void eap_handler_free(rlm_eap_t *inst, eap_handler_t *handler)
@@ -136,17 +135,13 @@ typedef struct check_handler_t {
        int             trips;
 } check_handler_t;
 
-static void check_handler(void *data)
+static int check_opaque_free(check_handler_t *check)
 {
        int do_warning = false;
        uint8_t state[8];
-       check_handler_t *check = data;
-
-       if (!check) return;
 
        if (!check->inst || !check->handler) {
-               free(check);
-               return;
+               return 0;
        }
 
        if (!check->inst->handler_tree) goto done;
@@ -184,7 +179,6 @@ static void check_handler(void *data)
 
 done:
        PTHREAD_MUTEX_UNLOCK(&(check->inst->handler_mutex));
-       free(check);
 
        if (do_warning) {
                WDEBUG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
@@ -197,6 +191,8 @@ done:
                WDEBUG("!! Please read http://wiki.freeradius.org/guide/Certificate_Compatibility     !!");
                WDEBUG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
        }
+
+       return 0;
 }
 
 void eaplist_free(rlm_eap_t *inst)
@@ -401,12 +397,14 @@ int eaplist_add(rlm_eap_t *inst, eap_handler_t *handler)
         *      Catch Access-Challenge without response.
         */
        if (inst->handler_tree) {
-               check_handler_t *check = rad_malloc(sizeof(*check));
+               check_handler_t *check = talloc(handler, check_handler_t);
 
                check->inst = inst;
                check->handler = handler;
                check->trips = handler->trips;
-               request_data_add(request, inst, 0, check, check_handler);
+
+               talloc_set_destructor(check, check_opaque_free);
+               request_data_add(request, inst, 0, check, true);
        }
 
        if (status) {

diff --git a/src/modules/rlm_eap/rlm_eap.c b/src/modules/rlm_eap/rlm_eap.c
index a72b809..2cad5cb 100644 (file)
--- a/src/modules/rlm_eap/rlm_eap.c
+++ b/src/modules/rlm_eap/rlm_eap.c
@@ -323,9 +323,10 @@ static rlm_rcode_t mod_authenticate(void *instance, REQUEST *request)
                 *      send a response.
                 */
                handler->inst_holder = inst;
-               status = request_data_add(request,
-                                         inst, REQUEST_DATA_EAP_HANDLER,
-                                         handler, (void *) eap_opaque_free);
+
+               talloc_set_destructor(handler, eap_opaque_free);
+               status = request_data_add(request, inst, REQUEST_DATA_EAP_HANDLER, handler, true);
+
                rad_assert(status == 0);
                return RLM_MODULE_HANDLED;
        }
@@ -347,10 +348,10 @@ static rlm_rcode_t mod_authenticate(void *instance, REQUEST *request)
                 *      send a response.
                 */
                handler->inst_holder = inst;
-               status = request_data_add(request,
-                                         inst, REQUEST_DATA_EAP_HANDLER,
-                                         handler,
-                                         (void *) eap_opaque_free);
+
+               talloc_set_destructor(handler, eap_opaque_free);
+               status = request_data_add(request, inst, REQUEST_DATA_EAP_HANDLER, handler, true);
+
                rad_assert(status == 0);
 
                /*

diff --git a/src/modules/rlm_eap/rlm_eap.h b/src/modules/rlm_eap/rlm_eap.h
index e961729..1a329a4 100644 (file)
--- a/src/modules/rlm_eap/rlm_eap.h
+++ b/src/modules/rlm_eap/rlm_eap.h
@@ -105,7 +105,7 @@ eap_handler_t       *eap_handler(rlm_eap_t *inst, eap_packet_raw_t **eap_msg, REQUEST
 EAP_DS         *eap_ds_alloc(eap_handler_t *handler);
 eap_handler_t  *eap_handler_alloc(rlm_eap_t *inst);
 void           eap_ds_free(EAP_DS **eap_ds);
-void           eap_opaque_free(eap_handler_t *handler);
+int            eap_opaque_free(eap_handler_t *handler);
 void           eap_handler_free(rlm_eap_t *inst, eap_handler_t *handler);
 
 int            eaplist_add(rlm_eap_t *inst, eap_handler_t *handler);

diff --git a/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c b/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
index 94ce726..45da585 100644 (file)
--- a/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
+++ b/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
@@ -637,7 +637,7 @@ packet_ready:
                rcode = request_data_add(request,
                                         request->proxy,
                                         REQUEST_DATA_EAP_TUNNEL_CALLBACK,
-                                        tunnel, NULL);
+                                        tunnel, false);
                rad_assert(rcode == 0);
 
                /*

diff --git a/src/modules/rlm_eap/types/rlm_eap_peap/peap.c b/src/modules/rlm_eap/types/rlm_eap_peap/peap.c
index 0568149..529eb9a 100644 (file)
--- a/src/modules/rlm_eap/types/rlm_eap_peap/peap.c
+++ b/src/modules/rlm_eap/types/rlm_eap_peap/peap.c
@@ -677,16 +677,6 @@ static int eappeap_postproxy(eap_handler_t *handler, void *data)
        eaptls_fail(handler, 0);
        return 0;
 }
-
-/*
- *     Free a request.
- */
-static void my_request_free(void *data)
-{
-       REQUEST *request = (REQUEST *)data;
-
-       request_free(&request);
-}
 #endif
 
 
@@ -1158,7 +1148,7 @@ int eappeap_process(eap_handler_t *handler, tls_session_t *tls_session)
                        rcode = request_data_add(request,
                                                 request->proxy,
                                                 REQUEST_DATA_EAP_TUNNEL_CALLBACK,
-                                                tunnel, NULL);
+                                                tunnel, false);
                        rad_assert(rcode == 0);
 
                        /*
@@ -1175,10 +1165,10 @@ int eappeap_process(eap_handler_t *handler, tls_session_t *tls_session)
                                 *      So we associate the fake request with
                                 *      this request.
                                 */
-                               rcode = request_data_add(request,
-                                                        request->proxy,
+                               talloc_set_destructor(fake, request_opaque_free);
+                               rcode = request_data_add(request, request->proxy,
                                                         REQUEST_DATA_EAP_MSCHAP_TUNNEL_CALLBACK,
-                                                        fake, my_request_free);
+                                                        fake, true);
                                rad_assert(rcode == 0);
 
                                /*

diff --git a/src/modules/rlm_eap/types/rlm_eap_ttls/ttls.c b/src/modules/rlm_eap/types/rlm_eap_ttls/ttls.c
index 04f885a..0128623 100644 (file)
--- a/src/modules/rlm_eap/types/rlm_eap_ttls/ttls.c
+++ b/src/modules/rlm_eap/types/rlm_eap_ttls/ttls.c
@@ -910,16 +910,6 @@ static int eapttls_postproxy(eap_handler_t *handler, void *data)
        return 0;
 }
 
-
-/*
- *     Free a request.
- */
-static void my_request_free(void *data)
-{
-       REQUEST *request = (REQUEST *)data;
-
-       request_free(&request);
-}
 #endif /* WITH_PROXY */
 
 /*
@@ -1240,7 +1230,7 @@ int eapttls_process(eap_handler_t *handler, tls_session_t *tls_session)
                         *      Associate the callback with the request.
                         */
                        code = request_data_add(request, request->proxy, REQUEST_DATA_EAP_TUNNEL_CALLBACK,
-                                                  tunnel, NULL);
+                                               tunnel, false);
                        rad_assert(code == 0);
 
                        /*
@@ -1250,8 +1240,9 @@ int eapttls_process(eap_handler_t *handler, tls_session_t *tls_session)
                         *      So we associate the fake request with
                         *      this request.
                         */
+                       talloc_set_destructor(fake, request_opaque_free);
                        code = request_data_add(request, request->proxy, REQUEST_DATA_EAP_MSCHAP_TUNNEL_CALLBACK,
-                                                  fake, my_request_free);
+                                               fake, true);
                        rad_assert(code == 0);
                        fake = NULL;