[svn-commits] wdoekes: trunk r342930 - in /trunk: ./ channels/ configs/ include/asterisk/ m...
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Tue Nov 1 16:03:03 CDT 2011
Author: wdoekes
Date: Tue Nov 1 16:02:56 2011
New Revision: 342930
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=342930
Log:
Several fixes to the chan_sip dynamic realtime peer/user lookup
There were several problems with the dynamic realtime peer/user lookup
code. The lookup logic had become rather hard to read due to lots of
incremental changes to the realtime_peer function. And, during the
addition of the sipregs functionality, several possibilities for memory
leaks had been introduced. The insecure=port matching has always been
broken for anyone using the sipregs family. And, related, the broken
implementation forced those using sipregs to *still* have an ipaddr
column on their sippeers table.
Thanks Terry Wilson for comprehensive testing and finding and fixing
unexpected behaviour from the multientry realtime call which caused
the realtime_peer to have a completely unused code path.
This changeset fixes the leaks, the lookup inconsistenties and that
you won't need an ipaddr column on your sippeers table anymore (when
you're using sipregs). Beware that when you're using sipregs, peers
with insecure=port will now start matching!
(closes issue ASTERISK-17792)
(closes issue ASTERISK-18356)
Reported by: marcelloceschia, Walter Doekes
Reviewed by: Terry Wilson
Review: https://reviewboard.asterisk.org/r/1395
........
Merged revisions 342927 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........
Merged revisions 342929 from http://svn.asterisk.org/svn/asterisk/branches/10
Modified:
trunk/ (props changed)
trunk/channels/chan_sip.c
trunk/configs/extconfig.conf.sample
trunk/include/asterisk/config.h
trunk/main/config.c
Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-10-merged' - no diff available.
Modified: trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_sip.c?view=diff&rev=342930&r1=342929&r2=342930
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Tue Nov 1 16:02:56 2011
@@ -1448,7 +1448,7 @@
static void realtime_update_peer(const char *peername, struct ast_sockaddr *addr, const char *username, const char *fullcontact, const char *useragent, int expirey, unsigned short deprecated_username, int lastms);
static void update_peer(struct sip_peer *p, int expire);
static struct ast_variable *get_insecure_variable_from_config(struct ast_config *config);
-static const char *get_name_from_variable(struct ast_variable *var, const char *newpeername);
+static const char *get_name_from_variable(const struct ast_variable *var);
static struct sip_peer *realtime_peer(const char *peername, struct ast_sockaddr *sin, int devstate_only, int which_objects);
static char *sip_prune_realtime(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a);
@@ -4666,14 +4666,207 @@
return var;
}
-static const char *get_name_from_variable(struct ast_variable *var, const char *newpeername)
-{
- struct ast_variable *tmp;
+static struct ast_variable *get_insecure_variable_from_sippeers(const char *column, const char *value)
+{
+ struct ast_config *peerlist;
+ struct ast_variable *var = NULL;
+ if ((peerlist = ast_load_realtime_multientry("sippeers", column, value, "insecure LIKE", "%port%", SENTINEL))) {
+ if ((var = get_insecure_variable_from_config(peerlist))) {
+ /* Must clone, because var will get freed along with
+ * peerlist. */
+ var = ast_variables_dup(var);
+ }
+ ast_config_destroy(peerlist);
+ }
+ return var;
+}
+
+/* Yes.. the only column that makes sense to pass is "ipaddr", but for
+ * consistency's sake, we require the column name to be passed. As extra
+ * argument, we take a pointer to var. We already got the info, so we better
+ * return it and save the caller a query. If return value is nonzero, then *var
+ * is nonzero too (and the other way around). */
+static struct ast_variable *get_insecure_variable_from_sipregs(const char *column, const char *value, struct ast_variable **var)
+{
+ struct ast_variable *varregs = NULL;
+ struct ast_config *regs, *peers;
+ char *regscat;
+ const char *regname;
+
+ if (!(regs = ast_load_realtime_multientry("sipregs", column, value, SENTINEL))) {
+ return NULL;
+ }
+
+ /* Load *all* peers that are probably insecure=port */
+ if (!(peers = ast_load_realtime_multientry("sippeers", "insecure LIKE", "%port%", SENTINEL))) {
+ ast_config_destroy(regs);
+ return NULL;
+ }
+
+ /* Loop over the sipregs that match IP address and attempt to find an
+ * insecure=port match to it in sippeers. */
+ regscat = NULL;
+ while ((regscat = ast_category_browse(regs, regscat)) && (regname = ast_variable_retrieve(regs, regscat, "name"))) {
+ char *peerscat;
+ const char *peername;
+
+ peerscat = NULL;
+ while ((peerscat = ast_category_browse(peers, peerscat)) && (peername = ast_variable_retrieve(peers, peerscat, "name"))) {
+ if (!strcasecmp(regname, peername)) {
+ /* Ensure that it really is insecure=port and
+ * not something else. */
+ const char *insecure = ast_variable_retrieve(peers, peerscat, "insecure");
+ struct ast_flags flags = {0};
+ set_insecure_flags(&flags, insecure, -1);
+ if (ast_test_flag(&flags, SIP_INSECURE_PORT)) {
+ /* ENOMEM checks till the bitter end. */
+ if ((varregs = ast_variables_dup(ast_category_root(regs, regscat)))) {
+ if (!(*var = ast_variables_dup(ast_category_root(peers, peerscat)))) {
+ ast_variables_destroy(varregs);
+ varregs = NULL;
+ }
+ }
+ goto done;
+ }
+ }
+ }
+ }
+
+done:
+ ast_config_destroy(regs);
+ ast_config_destroy(peers);
+ return varregs;
+}
+
+static const char *get_name_from_variable(const struct ast_variable *var)
+{
+ const struct ast_variable *tmp;
for (tmp = var; tmp; tmp = tmp->next) {
- if (!newpeername && !strcasecmp(tmp->name, "name"))
- newpeername = tmp->value;
- }
- return newpeername;
+ if (!strcasecmp(tmp->name, "name")) {
+ return tmp->value;
+ }
+ }
+ return NULL;
+}
+
+/* If varregs is NULL, we don't use sipregs.
+ * Using empty if-bodies instead of goto's while avoiding unnecessary indents */
+static int realtime_peer_by_name(const char *const *name, struct ast_sockaddr *addr, const char *ipaddr, struct ast_variable **var, struct ast_variable **varregs)
+{
+ /* Peer by name and host=dynamic */
+ if ((*var = ast_load_realtime("sippeers", "name", *name, "host", "dynamic", SENTINEL))) {
+ ;
+ /* Peer by name and host=IP */
+ } else if (addr && !(*var = ast_load_realtime("sippeers", "name", *name, "host", ipaddr, SENTINEL))) {
+ ;
+ /* Peer by name and host=HOSTNAME */
+ } else if ((*var = ast_load_realtime("sippeers", "name", *name, SENTINEL))) {
+ /*!\note
+ * If this one loaded something, then we need to ensure that the host
+ * field matched. The only reason why we can't have this as a criteria
+ * is because we only have the IP address and the host field might be
+ * set as a name (and the reverse PTR might not match).
+ */
+ if (addr) {
+ struct ast_variable *tmp;
+ for (tmp = *var; tmp; tmp = tmp->next) {
+ if (!strcasecmp(tmp->name, "host")) {
+ struct ast_sockaddr *addrs = NULL;
+
+ if (ast_sockaddr_resolve(&addrs,
+ tmp->value,
+ PARSE_PORT_FORBID,
+ get_address_family_filter(&bindaddr)) <= 0 ||
+ ast_sockaddr_cmp(&addrs[0], addr)) {
+ /* No match */
+ ast_variables_destroy(*var);
+ *var = NULL;
+ }
+ ast_free(addrs);
+ break;
+ }
+ }
+ }
+ }
+
+ /* Did we find anything? */
+ if (*var) {
+ if (varregs) {
+ *varregs = ast_load_realtime("sipregs", "name", *name, SENTINEL);
+ }
+ return 1;
+ }
+ return 0;
+}
+
+/* Another little helper function for backwards compatibility: this
+ * checks/fetches the sippeer that belongs to the sipreg. If none is
+ * found, we free the sipreg and return false. This way we can do the
+ * check inside the if-condition below. In the old code, not finding
+ * the sippeer also had it continue look for another match, so we do
+ * the same. */
+static struct ast_variable *realtime_peer_get_sippeer_helper(const char **name, struct ast_variable **varregs) {
+ struct ast_variable *var;
+ const char *old_name = *name;
+ *name = get_name_from_variable(*varregs);
+ if (!(var = ast_load_realtime("sippeers", "name", *name, SENTINEL))) {
+ ast_variables_destroy(*varregs);
+ *varregs = NULL;
+ *name = old_name;
+ }
+ return var;
+}
+
+/* If varregs is NULL, we don't use sipregs. If we return true, then *name is
+ * set. Using empty if-bodies instead of goto's while avoiding unnecessary
+ * indents. */
+static int realtime_peer_by_addr(const char **name, struct ast_sockaddr *addr, const char *ipaddr, struct ast_variable **var, struct ast_variable **varregs)
+{
+ char portstring[6]; /* up to 5 digits plus null terminator */
+ ast_copy_string(portstring, ast_sockaddr_stringify_port(addr), sizeof(portstring));
+
+ /* We're not finding this peer by this name anymore. Reset it. */
+ *name = NULL;
+
+ /* First check for fixed IP hosts */
+ if ((*var = ast_load_realtime("sippeers", "host", ipaddr, "port", portstring, SENTINEL))) {
+ ;
+ /* Check for registered hosts (in sipregs) */
+ } else if (varregs && (*varregs = ast_load_realtime("sipregs", "ipaddr", ipaddr, "port", portstring, SENTINEL)) &&
+ (*var = realtime_peer_get_sippeer_helper(name, varregs))) {
+ ;
+ /* Check for registered hosts (in sippeers) */
+ } else if (!varregs && (*var = ast_load_realtime("sippeers", "ipaddr", ipaddr, "port", portstring, SENTINEL))) {
+ ;
+ /* We couldn't match on ipaddress and port, so we need to check if port is insecure */
+ } else if ((*var = get_insecure_variable_from_sippeers("host", ipaddr))) {
+ ;
+ /* Same as above, but try the IP address field (in sipregs)
+ * Observe that it fetches the name/var at the same time, without the
+ * realtime_peer_get_sippeer_helper. Also note that it is quite inefficient.
+ * Avoid sipregs if possible. */
+ } else if (varregs && (*varregs = get_insecure_variable_from_sipregs("ipaddr", ipaddr, var))) {
+ ;
+ /* Same as above, but try the IP address field (in sippeers) */
+ } else if (!varregs && (*var = get_insecure_variable_from_sippeers("ipaddr", ipaddr))) {
+ ;
+ }
+
+ /* Did we find anything? */
+ if (*var) {
+ /* Make sure name is populated. */
+ if (!*name) {
+ *name = get_name_from_variable(*var);
+ }
+ /* Make sure varregs is populated if var is. Ensuring
+ * that var is set when varregs is, is taken care of by
+ * realtime_peer_get_sippeer_helper(). */
+ if (varregs && !*varregs) {
+ *varregs = ast_load_realtime("sipregs", "name", *name, SENTINEL);
+ }
+ return 1;
+ }
+ return 0;
}
/*! \brief realtime_peer: Get peer from realtime storage
@@ -4681,162 +4874,48 @@
* Checks the "sipregs" realtime family from extconfig.conf if it's configured.
* This returns a pointer to a peer and because we use build_peer, we can rest
* assured that the refcount is bumped.
-*/
+ *
+ * \note This is never called with both newpeername and addr at the same time.
+ * If you do, be prepared to get a peer with a different name than newpeername.
+ */
static struct sip_peer *realtime_peer(const char *newpeername, struct ast_sockaddr *addr, int devstate_only, int which_objects)
{
- struct sip_peer *peer;
+ struct sip_peer *peer = NULL;
struct ast_variable *var = NULL;
struct ast_variable *varregs = NULL;
- struct ast_variable *tmp;
- struct ast_config *peerlist = NULL;
char ipaddr[INET6_ADDRSTRLEN];
- char portstring[6]; /*up to 5 digits plus null terminator*/
int realtimeregs = ast_check_realtime("sipregs");
- /* First check on peer name */
- if (newpeername) {
- if (realtimeregs)
- varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
-
- var = ast_load_realtime("sippeers", "name", newpeername, "host", "dynamic", SENTINEL);
- if (!var && addr) {
- var = ast_load_realtime("sippeers", "name", newpeername, "host", ast_sockaddr_stringify_addr(addr), SENTINEL);
- }
- if (!var) {
- var = ast_load_realtime("sippeers", "name", newpeername, SENTINEL);
- /*!\note
- * If this one loaded something, then we need to ensure that the host
- * field matched. The only reason why we can't have this as a criteria
- * is because we only have the IP address and the host field might be
- * set as a name (and the reverse PTR might not match).
- */
- if (var && addr) {
- for (tmp = var; tmp; tmp = tmp->next) {
- if (!strcasecmp(tmp->name, "host")) {
- struct ast_sockaddr *addrs = NULL;
-
- if (ast_sockaddr_resolve(&addrs,
- tmp->value,
- PARSE_PORT_FORBID,
- get_address_family_filter(&bindaddr)) <= 0 ||
- ast_sockaddr_cmp(&addrs[0], addr)) {
- /* No match */
- ast_variables_destroy(var);
- var = NULL;
- }
- ast_free(addrs);
- break;
- }
- }
- }
- }
- }
-
- if (!var && addr) { /* Then check on IP address for dynamic peers */
+ if (addr) {
ast_copy_string(ipaddr, ast_sockaddr_stringify_addr(addr), sizeof(ipaddr));
- ast_copy_string(portstring, ast_sockaddr_stringify_port(addr), sizeof(portstring));
- var = ast_load_realtime("sippeers", "host", ipaddr, "port", portstring, SENTINEL); /* First check for fixed IP hosts */
- if (var) {
- if (realtimeregs) {
- newpeername = get_name_from_variable(var, newpeername);
- varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
- }
- } else {
- if (realtimeregs)
- varregs = ast_load_realtime("sipregs", "ipaddr", ipaddr, "port", portstring, SENTINEL); /* Then check for registered hosts */
- else
- var = ast_load_realtime("sippeers", "ipaddr", ipaddr, "port", portstring, SENTINEL); /* Then check for registered hosts */
- if (varregs) {
- newpeername = get_name_from_variable(varregs, newpeername);
- var = ast_load_realtime("sippeers", "name", newpeername, SENTINEL);
- }
- }
- if (!var) { /*We couldn't match on ipaddress and port, so we need to check if port is insecure*/
- peerlist = ast_load_realtime_multientry("sippeers", "host", ipaddr, SENTINEL);
- if (peerlist) {
- var = get_insecure_variable_from_config(peerlist);
- if(var) {
- if (realtimeregs) {
- newpeername = get_name_from_variable(var, newpeername);
- varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
- }
- } else { /*var wasn't found in the list of "hosts", so try "ipaddr"*/
- peerlist = NULL;
- peerlist = ast_load_realtime_multientry("sippeers", "ipaddr", ipaddr, SENTINEL);
- if(peerlist) {
- var = get_insecure_variable_from_config(peerlist);
- if(var) {
- if (realtimeregs) {
- newpeername = get_name_from_variable(var, newpeername);
- varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
- }
- }
- }
- }
- } else {
- if (realtimeregs) {
- peerlist = ast_load_realtime_multientry("sipregs", "ipaddr", ipaddr, SENTINEL);
- if (peerlist) {
- varregs = get_insecure_variable_from_config(peerlist);
- if (varregs) {
- newpeername = get_name_from_variable(varregs, newpeername);
- var = ast_load_realtime("sippeers", "name", newpeername, SENTINEL);
- }
- }
- } else {
- peerlist = ast_load_realtime_multientry("sippeers", "ipaddr", ipaddr, SENTINEL);
- if (peerlist) {
- var = get_insecure_variable_from_config(peerlist);
- if (var) {
- newpeername = get_name_from_variable(var, newpeername);
- varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
- }
- }
- }
- }
- }
- }
-
- if (!var) {
- if (peerlist)
- ast_config_destroy(peerlist);
+ } else {
+ ipaddr[0] = '\0';
+ }
+
+ if (newpeername && realtime_peer_by_name(&newpeername, addr, ipaddr, &var, realtimeregs ? &varregs : NULL)) {
+ ;
+ } else if (addr && realtime_peer_by_addr(&newpeername, addr, ipaddr, &var, realtimeregs ? &varregs : NULL)) {
+ ;
+ } else {
+ ast_log(LOG_WARNING, "Cannot Determine peer name ip=%s\n", ipaddr);
return NULL;
}
- for (tmp = var; tmp; tmp = tmp->next) {
- if (!strcasecmp(tmp->name, "type") && (!strcasecmp(tmp->value, "peer") && which_objects == FINDUSERS)) {
- if (peerlist) {
- ast_config_destroy(peerlist);
- } else {
- ast_variables_destroy(var);
- ast_variables_destroy(varregs);
- }
- return NULL;
- } else if (!newpeername && !strcasecmp(tmp->name, "name")) {
- newpeername = tmp->value;
- }
- }
-
- if (!newpeername) { /* Did not find peer in realtime */
- ast_log(LOG_WARNING, "Cannot Determine peer name ip=%s\n", ipaddr);
- if(peerlist)
- ast_config_destroy(peerlist);
- else
- ast_variables_destroy(var);
- return NULL;
- }
-
+ /* If we're looking for users, don't return peers (although this check
+ * should probably be done in realtime_peer_by_* instead...) */
+ if (which_objects == FINDUSERS) {
+ struct ast_variable *tmp;
+ for (tmp = var; tmp; tmp = tmp->next) {
+ if (!strcasecmp(tmp->name, "type") && (!strcasecmp(tmp->value, "peer"))) {
+ goto cleanup;
+ }
+ }
+ }
/* Peer found in realtime, now build it in memory */
peer = build_peer(newpeername, var, varregs, TRUE, devstate_only);
if (!peer) {
- if(peerlist)
- ast_config_destroy(peerlist);
- else {
- ast_variables_destroy(var);
- ast_variables_destroy(varregs);
- }
- return NULL;
+ goto cleanup;
}
ast_debug(3, "-REALTIME- loading peer from database to memory. Name: %s. Peer objects: %d\n", peer->name, rpeerobjs);
@@ -4856,13 +4935,10 @@
}
}
peer->is_realtime = 1;
- if (peerlist)
- ast_config_destroy(peerlist);
- else {
- ast_variables_destroy(var);
- ast_variables_destroy(varregs);
- }
-
+
+cleanup:
+ ast_variables_destroy(var);
+ ast_variables_destroy(varregs);
return peer;
}
Modified: trunk/configs/extconfig.conf.sample
URL: http://svnview.digium.com/svn/asterisk/trunk/configs/extconfig.conf.sample?view=diff&rev=342930&r1=342929&r2=342930
==============================================================================
--- trunk/configs/extconfig.conf.sample (original)
+++ trunk/configs/extconfig.conf.sample Tue Nov 1 16:02:56 2011
@@ -64,7 +64,7 @@
;iaxusers => odbc,asterisk
;iaxpeers => odbc,asterisk
;sippeers => odbc,asterisk
-;sipregs => odbc,asterisk
+;sipregs => odbc,asterisk ; (avoid sipregs if possible, e.g. by using a view)
;voicemail => odbc,asterisk
;extensions => odbc,asterisk
;meetme => mysql,general
Modified: trunk/include/asterisk/config.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/config.h?view=diff&rev=342930&r1=342929&r2=342930
==============================================================================
--- trunk/include/asterisk/config.h (original)
+++ trunk/include/asterisk/config.h Tue Nov 1 16:02:56 2011
@@ -448,6 +448,17 @@
/*! \brief Check if there's any realtime engines loaded */
int ast_realtime_enabled(void);
+
+/*!
+ * \brief Duplicate variable list
+ * \param var the linked list of variables to clone
+ * \return A duplicated list which you'll need to free with
+ * ast_variables_destroy or NULL when out of memory.
+ *
+ * \note Do not depend on this to copy more than just name, value and filename
+ * (the arguments to ast_variables_new).
+ */
+struct ast_variable *ast_variables_dup(struct ast_variable *var);
/*!
* \brief Free variable list
Modified: trunk/main/config.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/config.c?view=diff&rev=342930&r1=342929&r2=342930
==============================================================================
--- trunk/main/config.c (original)
+++ trunk/main/config.c Tue Nov 1 16:02:56 2011
@@ -523,6 +523,28 @@
ast_comment_destroy(&doomed->sameline);
ast_comment_destroy(&doomed->trailing);
ast_free(doomed);
+}
+
+struct ast_variable *ast_variables_dup(struct ast_variable *var)
+{
+ struct ast_variable *cloned;
+ struct ast_variable *tmp;
+
+ if (!(cloned = ast_variable_new(var->name, var->value, var->file))) {
+ return NULL;
+ }
+
+ tmp = cloned;
+
+ while ((var = var->next)) {
+ if (!(tmp->next = ast_variable_new(var->name, var->value, var->file))) {
+ ast_variables_destroy(cloned);
+ return NULL;
+ }
+ tmp = tmp->next;
+ }
+
+ return cloned;
}
void ast_variables_destroy(struct ast_variable *v)
More information about the svn-commits
mailing list