Change in libosmocore[master]: ctrl: Pre-calculate required size before allocating msgb

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Thu Jun 10 12:50:46 UTC 2021


pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/24639 )


Change subject: ctrl: Pre-calculate required size before allocating msgb
......................................................................

ctrl: Pre-calculate required size before allocating msgb

This commit fixes crash when response is more than ~4096 chars.
Furthermore, we now allocate only the required memory, not 4096 for all
messages, which usually don't require it.
Test needs to be adapted since it assumed there was more available space
at the end of the msgb.

Related: OS#5169
Change-Id: I0b8f370f7b08736207f9efed13a0663b5e482824
---
M src/ctrl/control_cmd.c
M tests/ctrl/ctrl_test.c
2 files changed, 34 insertions(+), 62 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/39/24639/1

diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index 33496bd..753e6cc 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -516,92 +516,61 @@
  *  \returns callee-allocated message buffer containing the encoded \a cmd; NULL on error */
 struct msgb *ctrl_cmd_make(struct ctrl_cmd *cmd)
 {
-	struct msgb *msg;
+	struct msgb *msg = NULL;
+	char *strbuf;
+	size_t len;
 	const char *type;
-	char *tmp;
 
 	if (!cmd->id)
 		return NULL;
 
-	msg = msgb_alloc_headroom(4096, 128, "ctrl command make");
-	if (!msg)
-		return NULL;
-
 	type = get_value_string(ctrl_type_vals, cmd->type);
 
 	switch (cmd->type) {
 	case CTRL_TYPE_GET:
 		if (!cmd->variable)
-			goto err;
-
-		tmp = talloc_asprintf(cmd, "%s %s %s", type, cmd->id, cmd->variable);
-		if (!tmp) {
-			LOGP(DLCTRL, LOGL_ERROR, "Failed to allocate cmd.\n");
-			goto err;
-		}
-
-		msg->l2h = msgb_put(msg, strlen(tmp));
-		memcpy(msg->l2h, tmp, strlen(tmp));
-		talloc_free(tmp);
+			return NULL;
+		strbuf = talloc_asprintf(cmd, "%s %s %s", type, cmd->id, cmd->variable);
 		break;
 	case CTRL_TYPE_SET:
 		if (!cmd->variable || !cmd->value)
-			goto err;
-
-		tmp = talloc_asprintf(cmd, "%s %s %s %s", type, cmd->id, cmd->variable,
-				cmd->value);
-		if (!tmp) {
-			LOGP(DLCTRL, LOGL_ERROR, "Failed to allocate cmd.\n");
-			goto err;
-		}
-
-		msg->l2h = msgb_put(msg, strlen(tmp));
-		memcpy(msg->l2h, tmp, strlen(tmp));
-		talloc_free(tmp);
+			return NULL;
+		strbuf = talloc_asprintf(cmd, "%s %s %s %s", type, cmd->id,
+					 cmd->variable, cmd->value);
 		break;
 	case CTRL_TYPE_GET_REPLY:
 	case CTRL_TYPE_SET_REPLY:
 	case CTRL_TYPE_TRAP:
 		if (!cmd->variable || !cmd->reply)
-			goto err;
-
-		tmp = talloc_asprintf(cmd, "%s %s %s %s", type, cmd->id, cmd->variable,
-				cmd->reply);
-		if (!tmp) {
-			LOGP(DLCTRL, LOGL_ERROR, "Failed to allocate cmd.\n");
-			goto err;
-		}
-
-		msg->l2h = msgb_put(msg, strlen(tmp));
-		memcpy(msg->l2h, tmp, strlen(tmp));
-		talloc_free(tmp);
+			return NULL;
+		strbuf = talloc_asprintf(cmd, "%s %s %s %s", type, cmd->id,
+					 cmd->variable, cmd->reply);
 		break;
 	case CTRL_TYPE_ERROR:
 		if (!cmd->reply)
-			goto err;
-
-		tmp = talloc_asprintf(cmd, "%s %s %s", type, cmd->id,
-				cmd->reply);
-		if (!tmp) {
-			LOGP(DLCTRL, LOGL_ERROR, "Failed to allocate cmd.\n");
-			goto err;
-		}
-
-		msg->l2h = msgb_put(msg, strlen(tmp));
-		memcpy(msg->l2h, tmp, strlen(tmp));
-		talloc_free(tmp);
+			return NULL;
+		strbuf = talloc_asprintf(cmd, "%s %s %s", type, cmd->id, cmd->reply);
 		break;
 	default:
 		LOGP(DLCTRL, LOGL_NOTICE, "Unknown command type %i\n", cmd->type);
-		goto err;
-		break;
+		return NULL;
 	}
 
-	return msg;
+	if (!strbuf) {
+		LOGP(DLCTRL, LOGL_ERROR, "Failed to allocate cmd.\n");
+		goto ret;
+	}
+	len = strlen(strbuf);
 
-err:
-	msgb_free(msg);
-	return NULL;
+	msg = msgb_alloc_headroom(len + 128, 128, "ctrl ERROR command make");
+	if (!msg)
+		goto ret;
+	msg->l2h = msgb_put(msg, len);
+	memcpy(msg->l2h, strbuf, len);
+
+ret:
+	talloc_free(strbuf);
+	return msg;
 }
 
 /*! Build a deferred control command state and keep it the per-connection list of deferred commands.
diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c
index b46e9ac..01fb9f7 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -117,11 +117,14 @@
 	} else {
 		struct msgb *sent_msg = msgb_dequeue(&ccon->write_queue.msg_queue);
 		OSMO_ASSERT(sent_msg);
-		msgb_put_u8(sent_msg, 0);
 
-		printf("replied: '%s'\n", osmo_escape_str((char*)msgb_l2(sent_msg), -1));
+		char *strbuf = talloc_size(sent_msg, msgb_l2len(sent_msg) + 1);
+		memcpy(strbuf, msgb_l2(sent_msg), msgb_l2len(sent_msg));
+		strbuf[msgb_l2len(sent_msg)] = '\0';
+
+		printf("replied: '%s'\n", osmo_escape_str(strbuf, -1));
 		OSMO_ASSERT(t->reply_str);
-		OSMO_ASSERT(!strcmp(t->reply_str, (char*)msgb_l2(sent_msg)));
+		OSMO_ASSERT(!strcmp(t->reply_str, strbuf));
 		msgb_free(sent_msg);
 	}
 	osmo_wqueue_clear(&ccon->write_queue);

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/24639
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0b8f370f7b08736207f9efed13a0663b5e482824
Gerrit-Change-Number: 24639
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210610/6be61668/attachment.htm>


More information about the gerrit-log mailing list