@henningw commented on this pull request. Thanks for the update. I just did a quick review, focus on the usual string handling and memory allocation topics. pkg_free(ptr); - ptr = NULL; + *held = NULL; +} + +/* + * lost_copy_string(str, int*) { Have you considered using the existing ut.h pkg_str_dup(..) function? + curl = httpapi.http_connect(_m, &con, NULL, &res, mtheld, &que); + } else { + /* we have no connection ... do a NAPTR lookup */ + if(lost_parse_host(did.s, &host, &flag) > 0) { + + LM_DBG("no conn. trying NATPR lookup [%.*s]\n", host.len, host.s); + + /* remove '[' and ']' from string (IPv6) */ + if(flag == AF_INET6) { + host.s++; + host.len = host.len - 2; + } + /* is it a name or ip ... check nameinfo (reverse lookup) */ + len = 0; + ipstr = lost_copy_string(host, &len); Its necessary to make a copy here, can not the host.s be used? + goto err; + } + } else { + LM_ERR("failed to get location service for [%.*s]\n", did.len, + did.s); + lost_free_string(&que); /* clean up */ + lost_free_string(&idhdr); + goto err; + } + + LM_DBG("NATPR lookup returned [%.*s]\n", url.len, url.s); + + /* curl doesn't like str */ + len = 0; + lisurl = lost_copy_string(url, &len); Its necessary to make a copy here, can not the url.s be used? + /* clean up */ + if(rtype != NULL) { + pkg_free(rtype); + } + + if(heldreq != NULL && len == 0) { + LM_ERR("could not create POST request\n"); + goto err; + } + + LM_DBG("POST request: [%.*s]\n", len, heldreq); + + /* curl doesn't like str */ + len = 0; + lisurl = lost_copy_string(url, &len); same as above goto err; } LM_DBG("findService request: [%.*s]\n", req.len, req.s); /* send findService request to mapping server - HTTP POST */ - curlres = httpapi.http_connect(_m, &con, NULL, &ret, mtlost, &req); + if(naptr) { + /* copy url */ + len = 0; + urlrep = lost_copy_string(url, &len); same - /* get the error patterm */ - err.s = lost_get_childname(root, errors_element, &err.len); - LM_DBG("findService error response: [%.*s]\n", err.len, err.s); - if(err.len == 0) { - LM_ERR("error pattern element not found: [%.*s]\n", ret.len, ret.s); - /* free memory */ - lost_free_string(&ret); - goto err; + switch(fsrdata->category) { + case RESPONSE: + if(fsrdata->uri != NULL) { + /* get the first uri element */ + if((tmp.s = fsrdata->uri->value) != NULL) { + tmp.len = strlen(fsrdata->uri->value); + uri.s = lost_copy_string(tmp, &uri.len); if you need a str anyway, you could use pkg_str_dup + if(fsrdata->uri != NULL) { + /* get the first uri element */ + if((tmp.s = fsrdata->uri->value) != NULL) { + tmp.len = strlen(fsrdata->uri->value); + uri.s = lost_copy_string(tmp, &uri.len); + } + } else { + LM_ERR("uri not found: [%.*s]\n", ret.len, ret.s); + goto err; + } + if(fsrdata->mapping != NULL) { + /* get the displayName element */ + if((tmp.s = fsrdata->mapping->name->text) != NULL) { + tmp.len = strlen(fsrdata->mapping->name->text); + name.s = lost_copy_string(tmp, &name.len); if you need a str anyway, you could use pkg_str_dup + name.s = lost_copy_string(tmp, &name.len); + } + } else { + LM_ERR("name not found: [%.*s]\n", ret.len, ret.s); + goto err; + } + /* we are done */ + redirect = 0; + break; + case ERROR: + /* get the errors element */ + if(fsrdata->errors != NULL) { + if((tmp.s = fsrdata->errors->issue->type) != NULL) { + tmp.len = strlen(fsrdata->errors->issue->type); + err.s = lost_copy_string(tmp, &err.len); if you need a str anyway, you could use pkg_str_dup + } + /* clean up */ + tmp.s = NULL; + tmp.len = 0; + /* check loop */ + if(oldurl.s != NULL && oldurl.len > 0) { + if(str_strcasecmp(&url, &oldurl) == 0) { + LM_ERR("loop detected: " + "[%.*s]<-->[%.*s]\n", + oldurl.len, oldurl.s, url.len, url.s); + goto err; + } + } + /* remember the redirect target */ + oldurl.s = lost_copy_string(url, &oldurl.len); if you need a str anyway, you can use pkg_str_dup + if(str_strcasecmp(&url, &oldurl) == 0) { + LM_ERR("loop detected: " + "[%.*s]<-->[%.*s]\n", + oldurl.len, oldurl.s, url.len, url.s); + goto err; + } + } + /* remember the redirect target */ + oldurl.s = lost_copy_string(url, &oldurl.len); + /* clean up */ + lost_free_findServiceResponse(&fsrdata); + lost_free_string(&ret); + /* copy url */ + len = 0; + urlrep = lost_copy_string(url, &len); same + LM_WARN("no response type found\n"); + rtype = NULL; + } else { + len = 0; + /* response type string sanity check */ + rtype = lost_held_type(rtp.s, &exact, &len); + if(len == 0) { + LM_WARN("cannot normalize [%.*s]\n", rtp.len, rtp.s); + rtype = NULL; + } + } + } + + /* default responseTime: emergencyRouting */ + heldreq = lost_held_post_request(&len, 0, rtype); this will allocate pkg memory to heldreq + /* response type string sanity check */ + rtype = lost_held_type(rtp.s, &exact, &len); + if(len == 0) { + LM_WARN("cannot normalize [%.*s]\n", rtp.len, rtp.s); + rtype = NULL; + } + } + } + + /* default responseTime: emergencyRouting */ + heldreq = lost_held_post_request(&len, 0, rtype); + + /* responseTime: milliseconds */ + if((ltime > 0) && (strlen(ptr) == 0)) { + heldreq = lost_held_post_request(&len, ltime, rtype); and this again another chunk of pkg mem to heldreq, but only one is freed later one? + } + } + + /* default responseTime: emergencyRouting */ + heldreq = lost_held_post_request(&len, 0, rtype); + + /* responseTime: milliseconds */ + if((ltime > 0) && (strlen(ptr) == 0)) { + heldreq = lost_held_post_request(&len, ltime, rtype); + } + + /* responseTime: emergencyRouting|emergencyDispatch */ + if((ltime == 0) && (strlen(ptr) > 0)) { + if(strncasecmp(ptr, HELD_ED, strlen(HELD_ED)) == 0) { + heldreq = lost_held_post_request(&len, -1, rtype); see above + /* default responseTime: emergencyRouting */ + heldreq = lost_held_post_request(&len, 0, rtype); + + /* responseTime: milliseconds */ + if((ltime > 0) && (strlen(ptr) == 0)) { + heldreq = lost_held_post_request(&len, ltime, rtype); + } + + /* responseTime: emergencyRouting|emergencyDispatch */ + if((ltime == 0) && (strlen(ptr) > 0)) { + if(strncasecmp(ptr, HELD_ED, strlen(HELD_ED)) == 0) { + heldreq = lost_held_post_request(&len, -1, rtype); + } + if(strncasecmp(ptr, HELD_ER, strlen(HELD_ER)) == 0) { + heldreq = lost_held_post_request(&len, 0, rtype); see above +/* + * lost module naptr functions + * thankfully taken over from the enum module There are several functions (parse_naptr_regexp, naptr_greater, naptr_sort..) copied from the enum module. We could consider moving them to the core. If we want to keep them, the copyright from the author should be present here as well. +#define PATH_NODE_VIA (const char *)"via" +#define MAPP_NODE (const char *)"mapping" +#define MAPP_NODE_URI (const char *)"uri" +#define MAPP_PROP_EXP (const char *)"expires" +#define MAPP_PROP_LUP (const char *)"lastUpdated" +#define MAPP_PROP_SRC (const char *)"source" +#define MAPP_PROP_SID (const char *)"sourceId" + +#define RED_NODE (const char *)"redirect" +#define RED_PROP_TAR (const char *)"target" +#define RED_PROP_SRC (const char *)"source" +#define RED_PROP_MSG (const char *)"message" + +#define ERRORS_NODE (const char *)"errors" + The section below defines several data structures. In order to prevent overlapping with existing data structures in modules and other libraries you probably want to add a prefix (e.g. lost_list_t, lost_data_t etc..). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/2675#pullrequestreview-624954455