Hi,

I'm pretty sure that it's safe to free NULL pointer even in kamailio memory manager. So checking before pkg_free looks redundant. E.g.:
+               if (user.s != NULL)
+                       pkg_free(user.s);

2014-10-02 21:39 GMT+04:00 Hugh Waite <hugh.waite@acision.com>:
Module: sip-router
Branch: master
Commit: 44e29820a759405adb7657334e86ea474196e6fd
URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=44e29820a759405adb7657334e86ea474196e6fd

Author: Hugh Waite <hugh.waite@acision.com>
Committer: Hugh Waite <hugh.waite@acision.com>
Date:   Thu Oct  2 18:37:00 2014 +0100

rr: Fix memory leak when using outbound

- Flow token memory is freed after building the rr header

---

 modules/rr/record.c |   82 ++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/modules/rr/record.c b/modules/rr/record.c
index 4fda746..a521092 100644
--- a/modules/rr/record.c
+++ b/modules/rr/record.c
@@ -375,11 +375,12 @@ static int copy_flow_token(str *token, struct sip_msg *_m)
 int record_route(struct sip_msg* _m, str *params)
 {
        struct lump* l, *l2;
-       str user;
+       str user = {NULL, 0};
        struct to_body* from = NULL;
        str* tag;
        int use_ob = rr_obb.use_outbound ? rr_obb.use_outbound(_m) : 0;
        int sips;
+       int ret = 0;

        user.len = 0;

@@ -406,7 +407,8 @@ int record_route(struct sip_msg* _m, str *params)
        if (append_fromtag) {
                if (parse_from_header(_m) < 0) {
                        LM_ERR("From parsing failed\n");
-                       return -2;
+                       ret = -2;
+                       goto error;
                }
                from = (struct to_body*)_m->from->parsed;
                tag = &from->tag_value;
@@ -426,17 +428,20 @@ int record_route(struct sip_msg* _m, str *params)
                l2 = anchor_lump(_m, _m->headers->name.s - _m->buf, 0, 0);
                if (!l || !l2) {
                        LM_ERR("failed to create an anchor\n");
-                       return -5;
+                       ret = -5;
+                       goto error;
                }
                l = insert_cond_lump_after(l, COND_IF_DIFF_REALMS, 0);
                l2 = insert_cond_lump_before(l2, COND_IF_DIFF_REALMS, 0);
                if (!l || !l2) {
                        LM_ERR("failed to insert conditional lump\n");
-                       return -6;
+                       ret = -6;
+                       goto error;
                }
                if (build_rr(l, l2, &user, tag, params, OUTBOUND, sips) < 0) {
                        LM_ERR("failed to insert outbound Record-Route\n");
-                       return -7;
+                       ret = -7;
+                       goto error;
                }
        }

@@ -444,17 +449,24 @@ int record_route(struct sip_msg* _m, str *params)
        l2 = anchor_lump(_m, _m->headers->name.s - _m->buf, 0, 0);
        if (!l || !l2) {
                LM_ERR("failed to create an anchor\n");
-               return -3;
+               ret = -3;
+               goto error;
        }

        if (build_rr(l, l2, &user, tag, params, INBOUND, sips) < 0) {
                LM_ERR("failed to insert inbound Record-Route\n");
-               return -4;
+               ret = -4;
+               goto error;
        }

        /* reset the rr_param buffer */
        rr_param_buf.len = 0;
-       return 0;
+       ret = 0;
+error:
+       if ((use_ob == 1) || (use_ob == 2))
+               if (user.s != NULL)
+                       pkg_free(user.s);
+       return ret;
 }


@@ -470,7 +482,7 @@ int record_route(struct sip_msg* _m, str *params)
  */
 int record_route_preset(struct sip_msg* _m, str* _data)
 {
-       str user;
+       str user = {NULL, 0};
        struct to_body* from;
        struct lump* l;
        char* hdr, *p;
@@ -479,6 +491,7 @@ int record_route_preset(struct sip_msg* _m, str* _data)
        char *rr_prefix;
        int rr_prefix_len;
        int sips;
+       int ret = 0;

        sips = rr_is_sips(_m);
        if(sips==0) {
@@ -513,7 +526,8 @@ int record_route_preset(struct sip_msg* _m, str* _data)
        if (append_fromtag) {
                if (parse_from_header(_m) < 0) {
                        LM_ERR("From parsing failed\n");
-                       return -2;
+                       ret = -2;
+                       goto error;
                }
                from = (struct to_body*)_m->from->parsed;
        }
@@ -521,7 +535,8 @@ int record_route_preset(struct sip_msg* _m, str* _data)
        l = anchor_lump(_m, _m->headers->name.s - _m->buf, 0, HDR_RECORDROUTE_T);
        if (!l) {
                LM_ERR("failed to create lump anchor\n");
-               return -3;
+               ret = -3;
+               goto error;
        }

        hdr_len = rr_prefix_len;
@@ -544,7 +559,8 @@ int record_route_preset(struct sip_msg* _m, str* _data)
        hdr = pkg_malloc(hdr_len);
        if (!hdr) {
                LM_ERR("no pkg memory left\n");
-               return -4;
+               ret = -4;
+               goto error;
        }

        p = hdr;
@@ -581,9 +597,15 @@ int record_route_preset(struct sip_msg* _m, str* _data)
        if (!insert_new_lump_after(l, hdr, hdr_len, 0)) {
                LM_ERR("failed to insert new lump\n");
                pkg_free(hdr);
-               return -5;
+               ret = -5;
+               goto error;
        }
-       return 1;
+       ret = 1;
+error:
+       if ((use_ob == 1) || (use_ob == 2))
+               if (user.s != NULL)
+                       pkg_free(user.s);
+       return ret;
 }

 /*!
@@ -723,12 +745,13 @@ lump_err:

 int record_route_advertised_address(struct sip_msg* _m, str* _data)
 {
-       str user;
+       str user = {NULL, 0};
        str *tag = NULL;
        struct lump* l;
        struct lump* l2;
        int use_ob = rr_obb.use_outbound ? rr_obb.use_outbound(_m) : 0;
        int sips;
+       int ret = 0;

        user.len = 0;
        user.s = 0;
@@ -753,7 +776,8 @@ int record_route_advertised_address(struct sip_msg* _m, str* _data)
        if (append_fromtag) {
                if (parse_from_header(_m) < 0) {
                        LM_ERR("From parsing failed\n");
-                       return -2;
+                       ret = -2;
+                       goto error;
                }
                tag = &((struct to_body*)_m->from->parsed)->tag_value;
        }
@@ -765,18 +789,21 @@ int record_route_advertised_address(struct sip_msg* _m, str* _data)
                l2 = anchor_lump(_m, _m->headers->name.s - _m->buf, 0, 0);
                if (!l || !l2) {
                        LM_ERR("failed to create an anchor\n");
-                       return -3;
+                       ret = -3;
+                       goto error;
                }
                l = insert_cond_lump_after(l, COND_IF_DIFF_PROTO, 0);
                l2 = insert_cond_lump_before(l2, COND_IF_DIFF_PROTO, 0);
                if (!l || !l2) {
                        LM_ERR("failed to insert conditional lump\n");
-                       return -4;
+                       ret = -4;
+                       goto error;
                }
                if (build_advertised_rr(l, l2, _data, &user, tag, OUTBOUND,
                                        sips) < 0) {
                        LM_ERR("failed to insert outbound Record-Route\n");
-                       return -5;
+                       ret = -5;
+                       goto error;
                }
        }

@@ -784,14 +811,21 @@ int record_route_advertised_address(struct sip_msg* _m, str* _data)
        l2 = anchor_lump(_m, _m->headers->name.s - _m->buf, 0, 0);
        if (!l || !l2) {
                LM_ERR("failed to create an anchor\n");
-               return -6;
+               ret = -6;
+               goto error;
        }

        if (build_advertised_rr(l, l2, _data, &user, tag, INBOUND, sips) < 0) {
                LM_ERR("failed to insert outbound Record-Route\n");
-               return -7;
+               ret = -7;
+               goto error;
        }
-       return 1;
+       ret = 1;
+error:
+       if ((use_ob == 1) || (use_ob == 2))
+               if (user.s != NULL)
+                       pkg_free(user.s);
+       return ret;
 }




_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev



--
Best regards,
Alekzander Spiridonov