[sr-dev] git:3.2: modules_k/rls: Memory allocation problem for NOTIFY bodies

Module: sip-router Branch: 3.2 Commit: 30f6510eb8470e118984397e1944402a2198b05a URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=30f6510… Author: pd &lt;peter.dunkley(a)crocodile-rcs.com&gt; Committer: pd &lt;peter.dunkley(a)crocodile-rcs.com&gt; Date: Thu Dec 8 21:40:20 2011 +0000 modules_k/rls: Memory allocation problem for NOTIFY bodies - A few problems here: - The actual allocated buffer size was not used in the calculations in constr_multipart_body - The buffer pointer was copied to a local variable and not copied back after a realloc in constr_mulitpart_body(). This resulted in a double free when the realloc moved the buffer. - The length of the data in string buffer was never copied back to the str in constr_multipart_body(). - In both constr_multipart_body() and resource_notify.c when the buffer does not contain enough space another 2048 bytes is allocated. This is regardless of how much more space is actually needed. So if 4096 bytes were needed 2048 bytes would be allocated and then the end of the buffer would be overwritten by 4096 bytes of data. - Problem found and diagnosed during testing at Crocodile RCS - Fix implemented by Hugh Waite @ Crocodile RCS (cherry picked from commit 0f79902f0e44f0cc3c01607a917121fbce8d30a3) --- modules_k/rls/notify.c | 55 +++++++++++++++++++++++--------------- modules_k/rls/resource_notify.c | 2 +- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/modules_k/rls/notify.c b/modules_k/rls/notify.c index e964520..b9bad7f 100644 --- a/modules_k/rls/notify.c +++ b/modules_k/rls/notify.c @@ -51,6 +51,9 @@ #include <libxml/xpath.h> #include <libxml/xpathInternals.h> +static str *multipart_body = NULL; +static int multipart_body_size = 0; + typedef struct res_param { struct uri_link **next; @@ -68,7 +71,7 @@ int resource_uri_col=0, content_type_col, pres_state_col= 0, xmlDocPtr constr_rlmi_doc(db1_res_t* result, str* rl_uri, int version, xmlNodePtr rl_node, char*** cid_array, str username, str domain); -void constr_multipart_body(str *multipart_body, const str *const content_type, const str *const body, str *cid, int boundary_len, char *boundary_string); +void constr_multipart_body(const str *const content_type, const str *const body, str *cid, int boundary_len, char *boundary_string); dlg_t* rls_notify_dlg(subs_t* subs); @@ -78,14 +81,13 @@ int parse_xcap_uri(char *uri, str *host, unsigned short *port, str *path); int rls_get_resource_list(str *rl_uri, str *username, str *domain, xmlNodePtr *rl_node, xmlDocPtr *xmldoc); int add_resource_to_list(char* uri, void* param); -int add_resource(char* uri, xmlNodePtr list_node, str *multipart_body, char * boundary_string, db1_res_t *result, int *len_est); +int add_resource(char* uri, xmlNodePtr list_node, char * boundary_string, db1_res_t *result, int *len_est); int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri, unsigned int hash_code) { xmlDocPtr rlmi_body= NULL; xmlNodePtr list_node= NULL; - str* multipart_body= NULL; db_key_t query_cols[2], update_cols[2], result_cols[7]; db_val_t query_vals[2], update_vals[2]; db1_res_t *result= NULL; @@ -96,8 +98,6 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri, uri_link_t *uri_list_head = NULL; int len_est; res_param_t param; - char* body_buffer; - int size= BUF_REALLOC_SIZE; int resource_added = 0; /* Flag to indicate that we have added at least one resource */ LM_DBG("start\n"); @@ -137,19 +137,20 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri, /* Allocate an initial buffer for the multipart body. * This buffer will be reallocated if neccessary */ - body_buffer= (char *)pkg_malloc(size); - if(body_buffer== NULL) + multipart_body= (str*)pkg_malloc(sizeof(str)); + if(multipart_body== NULL) { ERR_MEM(PKG_MEM_STR); } - - multipart_body= (str*)pkg_malloc(sizeof(str)); - if(multipart_body== NULL) + + multipart_body_size = BUF_REALLOC_SIZE; + multipart_body->s = (char *)pkg_malloc(multipart_body_size); + + if(multipart_body->s== NULL) { ERR_MEM(PKG_MEM_STR); } - - multipart_body->s= body_buffer; + multipart_body->len= 0; /* Create an empty rlmi document */ @@ -169,7 +170,7 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri, while (uri_list_head) { uri_link_t *last = uri_list_head; - if (add_resource(uri_list_head->uri, list_node, multipart_body, boundary_string, result, &len_est) >0) + if (add_resource(uri_list_head->uri, list_node, boundary_string, result, &len_est) >0) { if (resource_added == 0) { @@ -257,9 +258,11 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri, if(multipart_body) { - pkg_free(multipart_body->s); + if (multipart_body->s) + pkg_free(multipart_body->s); pkg_free(multipart_body); } + multipart_body_size = 0; pkg_free(rlsubs_did.s); return 0; @@ -277,6 +280,7 @@ error: pkg_free(multipart_body->s); pkg_free(multipart_body); } + multipart_body_size = 0; if(result) rls_dbf.free_result(rls_db, result); @@ -357,7 +361,7 @@ error: int add_resource_instance(char* uri, xmlNodePtr resource_node, - db1_res_t* result, str *multipart_body, char * boundary_string, + db1_res_t* result, char * boundary_string, int *len_est) { xmlNodePtr instance_node= NULL; @@ -438,7 +442,7 @@ int add_resource_instance(char* uri, xmlNodePtr resource_node, if(auth_state_flag & ACTIVE_STATE) { - constr_multipart_body (multipart_body, &content_type, &body, &cid, boundary_len, boundary_string); + constr_multipart_body (&content_type, &body, &cid, boundary_len, boundary_string); xmlNewProp(instance_node, BAD_CAST "cid", BAD_CAST cid.s); } @@ -457,7 +461,7 @@ error: return -1; } -int add_resource(char* uri, xmlNodePtr list_node, str *multipart_body, char * boundary_string, db1_res_t *result, int *len_est) +int add_resource(char* uri, xmlNodePtr list_node, char * boundary_string, db1_res_t *result, int *len_est) { xmlNodePtr resource_node= NULL; int res; @@ -477,7 +481,7 @@ int add_resource(char* uri, xmlNodePtr list_node, str *multipart_body, char * bo } xmlNewProp(resource_node, BAD_CAST "uri", BAD_CAST uri); - res = add_resource_instance(uri, resource_node, result, multipart_body, boundary_string, len_est); + res = add_resource_instance(uri, resource_node, result, boundary_string, len_est); if(res < 0) { LM_ERR("while adding resource instance node\n"); @@ -575,10 +579,9 @@ error: } -void constr_multipart_body(str *multipart_body, const str *const content_type, const str *const body, str *cid, int boundary_len, char *boundary_string) +void constr_multipart_body(const str *const content_type, const str *const body, str *cid, int boundary_len, char *boundary_string) { char* buf= multipart_body->s; - int size= BUF_REALLOC_SIZE; int length= multipart_body->len; int chunk_len; @@ -589,10 +592,16 @@ void constr_multipart_body(str *multipart_body, const str *const content_type, c + 16 + cid->len + 18 + content_type->len + 4 + body->len + 8; - if(length + chunk_len >= size) + while(length + chunk_len >= multipart_body_size) { - REALLOC_BUF + multipart_body_size += BUF_REALLOC_SIZE; + multipart_body->s = (char*)pkg_realloc(multipart_body->s, multipart_body_size); + if(multipart_body->s == NULL) + { + ERR_MEM("constr_multipart_body"); + } } + buf = multipart_body->s; length+= sprintf(buf+ length, "--%.*s\r\n", boundary_len, boundary_string); @@ -604,6 +613,8 @@ void constr_multipart_body(str *multipart_body, const str *const content_type, c length+= sprintf(buf+length,"%.*s\r\n\r\n", body->len, body->s); + multipart_body->len = length; + error: return; diff --git a/modules_k/rls/resource_notify.c b/modules_k/rls/resource_notify.c index 978e01d..379b587 100644 --- a/modules_k/rls/resource_notify.c +++ b/modules_k/rls/resource_notify.c @@ -349,7 +349,7 @@ void send_notifies(db1_res_t *result, int did_col, int resource_uri_col, int aut if(cid.s) { - if(buf_len + chunk_len >= size) + while(buf_len + chunk_len >= size) { REALLOC_BUF }