[Asterisk-code-review] pbx.c: Simplify ast_context memory management. (asterisk[master])

Kevin Harwell asteriskteam at digium.com
Wed Jul 13 17:18:12 CDT 2022


Kevin Harwell has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/18749 )

Change subject: pbx.c: Simplify ast_context memory management.
......................................................................

pbx.c: Simplify ast_context memory management.

Allocate all of the ast_context's character data in the structure's
flexible array member and eliminate the clunky fake_context. This will
simplify future changes to ast_context.

Change-Id: I98357de75d8ac2b3c4c9f201223632e6901021ea
---
M main/pbx.c
1 file changed, 48 insertions(+), 49 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, approved; Approved for Submit



diff --git a/main/pbx.c b/main/pbx.c
index 3a701aa..c655f1b 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -280,21 +280,29 @@
 	struct ast_exten *exten;
 };
 
-/*! \brief ast_context: An extension context - must remain in sync with fake_context */
+/*! \brief ast_context: An extension context */
 struct ast_context {
-	ast_rwlock_t lock;			/*!< A lock to prevent multiple threads from clobbering the context */
-	struct ast_exten *root;			/*!< The root of the list of extensions */
-	struct ast_hashtab *root_table;            /*!< For exact matches on the extensions in the pattern tree, and for traversals of the pattern_tree  */
-	struct match_char *pattern_tree;        /*!< A tree to speed up extension pattern matching */
-	struct ast_context *next;		/*!< Link them together */
-	struct ast_includes includes;		/*!< Include other contexts */
-	struct ast_ignorepats ignorepats;	/*!< Patterns for which to continue playing dialtone */
-	struct ast_sws alts;			/*!< Alternative switches */
-	char *registrar;			/*!< Registrar -- make sure you malloc this, as the registrar may have to survive module unloads */
-	int refcount;                   /*!< each module that would have created this context should inc/dec this as appropriate */
-	int autohints;                  /*!< Whether autohints support is enabled or not */
-	ast_mutex_t macrolock;			/*!< A lock to implement "exclusive" macros - held whilst a call is executing in the macro */
-	char name[0];				/*!< Name of the context */
+	const char *name;
+	const char *registrar;
+
+	ast_rwlock_t lock;                /*!< A lock to prevent multiple threads from clobbering the context */
+	struct ast_exten *root;           /*!< The root of the list of extensions */
+	struct ast_hashtab *root_table;   /*!< For exact matches on the extensions in the pattern tree, and for traversals of the pattern_tree  */
+	struct match_char *pattern_tree;  /*!< A tree to speed up extension pattern matching */
+	struct ast_context *next;         /*!< Link them together */
+	struct ast_includes includes;     /*!< Include other contexts */
+	struct ast_ignorepats ignorepats; /*!< Patterns for which to continue playing dialtone */
+	struct ast_sws alts;              /*!< Alternative switches */
+	int refcount;                     /*!< each module that would have created this context should inc/dec this as appropriate */
+	int autohints;                    /*!< Whether autohints support is enabled or not */
+	ast_mutex_t macrolock;            /*!< A lock to implement "exclusive" macros - held whilst a call is executing in the macro */
+
+	/*!
+	 * Buffer to hold the name & registrar character data.
+	 *
+	 * The context name *must* be stored first in this buffer.
+	 */
+	char data[];
 };
 
 /*! \brief ast_state_cb: An extension state notify register item */
@@ -2429,35 +2437,18 @@
 	return extension_match_core(pattern, data, needmore);
 }
 
-/* This structure must remain in sync with ast_context for proper hashtab matching */
-struct fake_context /* this struct is purely for matching in the hashtab */
-{
-	ast_rwlock_t lock;
-	struct ast_exten *root;
-	struct ast_hashtab *root_table;
-	struct match_char *pattern_tree;
-	struct ast_context *next;
-	struct ast_includes includes;
-	struct ast_ignorepats ignorepats;
-	struct ast_sws alts;
-	const char *registrar;
-	int refcount;
-	int autohints;
-	ast_mutex_t macrolock;
-	char name[256];
-};
-
 struct ast_context *ast_context_find(const char *name)
 {
 	struct ast_context *tmp;
-	struct fake_context item;
+	struct ast_context item = {
+		.name = name,
+	};
 
 	if (!name) {
 		return NULL;
 	}
 	ast_rdlock_contexts();
 	if (contexts_table) {
-		ast_copy_string(item.name, name, sizeof(item.name));
 		tmp = ast_hashtab_lookup(contexts_table, &item);
 	} else {
 		tmp = NULL;
@@ -2768,7 +2759,10 @@
 			return NULL;
 		}
 	}
-	q->incstack[q->stacklen++] = tmp->name;	/* Setup the stack */
+	/* Technically we should be using tmp->name here, but if we used that we
+	 * would have to cast away the constness of the 'name' pointer and I do
+	 * not want to do that. */
+	q->incstack[q->stacklen++] = tmp->data;	/* Setup the stack */
 	/* Now try any includes we have in this context */
 	for (idx = 0; idx < ast_context_includes_count(tmp); idx++) {
 		const struct ast_include *i = ast_context_includes_get(tmp, idx);
@@ -4806,9 +4800,9 @@
  */
 static struct ast_context *find_context(const char *context)
 {
-	struct fake_context item;
-
-	ast_copy_string(item.name, context, sizeof(item.name));
+	struct ast_context item = {
+		.name = context,
+	};
 
 	return ast_hashtab_lookup(contexts_table, &item);
 }
@@ -4821,9 +4815,9 @@
 static struct ast_context *find_context_locked(const char *context)
 {
 	struct ast_context *c;
-	struct fake_context item;
-
-	ast_copy_string(item.name, context, sizeof(item.name));
+	struct ast_context item = {
+		.name = context,
+	};
 
 	ast_rdlock_contexts();
 	c = ast_hashtab_lookup(contexts_table, &item);
@@ -6199,8 +6193,12 @@
 struct ast_context *ast_context_find_or_create(struct ast_context **extcontexts, struct ast_hashtab *exttable, const char *name, const char *registrar)
 {
 	struct ast_context *tmp, **local_contexts;
-	struct fake_context search;
-	int length = sizeof(struct ast_context) + strlen(name) + 1;
+	struct ast_context search = {
+		.name = name,
+	};
+	size_t name_bytes = strlen(name);
+	size_t registrar_bytes = strlen(registrar);
+	int length = sizeof(struct ast_context) + name_bytes + registrar_bytes + 2;
 
 	if (!contexts_table) {
 		/* Protect creation of contexts_table from reentrancy. */
@@ -6216,7 +6214,6 @@
 		ast_unlock_contexts();
 	}
 
-	ast_copy_string(search.name, name, sizeof(search.name));
 	if (!extcontexts) {
 		ast_rdlock_contexts();
 		local_contexts = &contexts;
@@ -6238,14 +6235,19 @@
 	if ((tmp = ast_calloc(1, length))) {
 		ast_rwlock_init(&tmp->lock);
 		ast_mutex_init(&tmp->macrolock);
-		strcpy(tmp->name, name);
+		tmp->name = memcpy(&tmp->data[0], name, name_bytes);
+		tmp->registrar = memcpy(&tmp->data[name_bytes + 1], registrar, registrar_bytes);
 		tmp->root = NULL;
 		tmp->root_table = NULL;
-		tmp->registrar = ast_strdup(registrar);
 		AST_VECTOR_INIT(&tmp->includes, 0);
 		AST_VECTOR_INIT(&tmp->ignorepats, 0);
 		AST_VECTOR_INIT(&tmp->alts, 0);
 		tmp->refcount = 1;
+
+		/* The context 'name' must be stored at the beginning of 'data.' The
+		 * order of subsequent strings (currently only 'registrar') is not
+		 * relevant. */
+		ast_assert(tmp->name == &tmp->data[0]);
 	} else {
 		ast_log(LOG_ERROR, "Danger! We failed to allocate a context for %s!\n", name);
 		if (!extcontexts) {
@@ -8072,9 +8074,6 @@
 	AST_VECTOR_CALLBACK_VOID(&tmp->alts, sw_free);
 	AST_VECTOR_FREE(&tmp->alts);
 
-	if (tmp->registrar)
-		ast_free(tmp->registrar);
-
 	/* destroy the hash tabs */
 	if (tmp->root_table) {
 		ast_hashtab_destroy(tmp->root_table, 0);

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I98357de75d8ac2b3c4c9f201223632e6901021ea
Gerrit-Change-Number: 18749
Gerrit-PatchSet: 4
Gerrit-Owner: Sean Bright <sean at seanbright.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220713/7df7dc0f/attachment-0001.html>


More information about the asterisk-code-review mailing list