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

pespin gerrit-no-reply at lists.osmocom.org
Tue Jun 15 16:29:44 UTC 2021


pespin has submitted this change. ( 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(-)

Approvals:
  laforge: Looks good to me, but someone else must approve
  dexter: Looks good to me, but someone else must approve
  keith: Looks good to me, but someone else must approve
  pespin: Looks good to me, approved
  Jenkins Builder: Verified

Objections:
  neels: I would prefer this is not merged as is



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: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: keith <keith at rhizomatica.org>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210615/4805b5b2/attachment.htm>


More information about the gerrit-log mailing list