Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
@henningw commented on this pull request.
Thanks for the PR. I've done a review and added a few comments, mostly related to memroy management and error handling.
In src/modules/presence_reginfo/notify_body.c:
> + str *body = NULL;
+
+ xmlDocPtr doc = NULL;
+ xmlNodePtr root_node = NULL;
+ xmlNsPtr namespace = NULL;
+ xmlNodePtr p_root = NULL;
+ xmlDocPtr *xml_array;
+ xmlNodePtr node = NULL;
+ xmlNodePtr next_node = NULL;
+
+ LM_DBG("[pres_user]=%.*s [pres_domain]= %.*s, [n]=%d\n", pres_user->len,
+ pres_user->s, pres_domain->len, pres_domain->s, n);
+
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
You could use the PKG_MEM_ERROR here
In src/modules/presence_reginfo/notify_body.c:
> + /* The version must be increased for each new document and is a 32bit int.
+ * As the version is different for each watcher, we can not set here the
+ * correct value. Thus, we just put here a placeholder which will be
+ * replaced by the correct value in the aux_body_processing callback.
+ * Thus we have CPU intensive XML aggregation only once and can use
+ * quick search&replace in the per-watcher aux_body_processing callback.
+ * We use 11 chracters as an signed int (although RFC says unsigned int we
+ * use signed int as presence module stores "version" in DB as
+ * signed int) has max. 10 characters + 1 character for the sign
+ */
+ xmlNewProp(root_node, BAD_CAST "version", BAD_CAST "00000000000");
+ xmlNewProp(root_node, BAD_CAST "state", BAD_CAST "full");
+
+ /* loop over all bodies and create the aggregated body */
+ for(i = 0; i < j; i++) {
+ // LM_DBG("[n]=%d, [i]=%d, [j]=%d xml_array[i]=%p\n", n, i, j,
remove the commented out code if its not needed anymore
In src/modules/presence_reginfo/notify_body.c:
> +
+ /* we do not copy the node, but unlink it and then add it ot the new node
+ * this destroys the original document but we do not need it anyway.
+ */
+ xmlUnlinkNode(node);
+ if(xmlAddChild(root_node, node) == NULL) {
+ xmlFreeNode(node);
+ LM_ERR("while adding child\n");
+ goto error;
+ }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
What happened to the previously allocated memory in line 97?
In src/modules/presence_reginfo/notify_body.c:
> + * this destroys the original document but we do not need it anyway.
+ */
+ xmlUnlinkNode(node);
+ if(xmlAddChild(root_node, node) == NULL) {
+ xmlFreeNode(node);
+ LM_ERR("while adding child\n");
+ goto error;
+ }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
again PKG_MEM_ERROR
In src/modules/presence_reginfo/notify_body.c:
> + }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
+ return NULL;
+ }
+ memset(xml_array, 0, n * sizeof(xmlDocPtr));
+
+ body = (str *)pkg_malloc(sizeof(str));
+ if(body == NULL) {
+ ERR_MEM(PKG_MEM_STR);
You could also use PKG_MEM_ERROR, to have it everywhere the same
And you probably also want to exit and cleanup the previous allocated memory, right?
In src/modules/presence_reginfo/notify_body.c:
> + LM_ERR("body string too short!\n");
+ return NULL;
+ }
+ version_start = strstr(body->s + 30, "version=");
+ if(!version_start) {
+ LM_ERR("version string not found!\n");
+ return NULL;
+ }
+ version_start += 9;
+
+ /* safety check for placeholder - if it is body not set by the module,
+ * don't update the version */
+ if(strncmp(version_start, "00000000000\"", 12) != 0)
+ return NULL;
+
+ version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"", subs->version);
you probably also want to check for negative return (encoding error)
In src/modules/presence_reginfo/notify_body.c:
> + version_start += 9;
+
+ /* safety check for placeholder - if it is body not set by the module,
+ * don't update the version */
+ if(strncmp(version_start, "00000000000\"", 12) != 0)
+ return NULL;
+
+ version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"", subs->version);
+ if(version_len >= MAX_INT_LEN + 2) {
+ LM_ERR("failed to convert 'version' to string\n");
+ return NULL;
+ }
+
+ aux_body = (str *)pkg_malloc(sizeof(str));
+ if(aux_body == NULL) {
+ LM_ERR("error allocating memory for aux body str\n");
you could use PKG_MEM_ERROR or PKG_MEM_ERROR_FMT
In src/modules/presence_reginfo/notify_body.c:
> + version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"", subs->version);
+ if(version_len >= MAX_INT_LEN + 2) {
+ LM_ERR("failed to convert 'version' to string\n");
+ return NULL;
+ }
+
+ aux_body = (str *)pkg_malloc(sizeof(str));
+ if(aux_body == NULL) {
+ LM_ERR("error allocating memory for aux body str\n");
+ return NULL;
+ }
+ memset(aux_body, 0, sizeof(str));
+ aux_body->s = (char *)pkg_malloc(body->len * sizeof(char));
+ if(aux_body->s == NULL) {
+ pkg_free(aux_body);
+ LM_ERR("error allocating memory for aux body buffer\n");
see above
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.