[Asterisk-code-review] CDR: Alter destruction pattern for CDR chains. (asterisk[13])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/4151 )

Change subject: CDR: Alter destruction pattern for CDR chains.
......................................................................


CDR: Alter destruction pattern for CDR chains.

CDRs form chains. When the root of the chain is destroyed, it then
unreferences the next CDR in the chain. That CDR is destroyed, and it
then unreferences the next CDR in the chain. This repeats until the end
of the chain is reached. While this typically does not cause any sort of
problems, it is possible in strange scenarios for the CDR chain to grow
way longer than expected. In such a scenario, the destruction pattern
can result in a stack overflow.

This patch fixes the problem by switching from a recursive pattern to an
iterative pattern for destruction. When the root CDR is destroyed, it is
responsible for iterating over the rest of the CDRs and unreferencing
each one. Other CDRs in the chain, since they are not the root, will
simply destroy themselves and be done. This causes the stack depth not
to increase.

ASTERISK-26421 #close
Reported by Andrew Nagy

Change-Id: I3ca90c2b8051f3b7ead2e0e43f60d2c18fb204b8
---
M main/cdr.c
1 file changed, 18 insertions(+), 1 deletion(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/main/cdr.c b/main/cdr.c
index ab6530e..5e515d8 100644
--- a/main/cdr.c
+++ b/main/cdr.c
@@ -695,6 +695,7 @@
 	);
 	struct cdr_object *next;                /*!< The next CDR object in the chain */
 	struct cdr_object *last;                /*!< The last CDR object in the chain */
+	int is_root;                            /*!< True if this is the first CDR in the chain */
 };
 
 /*!
@@ -850,7 +851,22 @@
 	}
 	ast_string_field_free_memory(cdr);
 
-	ao2_cleanup(cdr->next);
+	/* CDR destruction used to work by calling ao2_cleanup(next) and
+	 * allowing the chain to destroy itself neatly. Unfortunately, for
+	 * really long chains, this can result in a stack overflow. So now
+	 * when the root CDR is destroyed, it is responsible for unreffing
+	 * all CDRs in the chain
+	 */
+	if (cdr->is_root) {
+		struct cdr_object *curr = cdr->next;
+		struct cdr_object *next;
+
+		while (curr) {
+			next = curr->next;
+			ao2_cleanup(curr);
+			curr = next;
+		}
+	}
 }
 
 /*!
@@ -2094,6 +2110,7 @@
 		if (!cdr) {
 			return;
 		}
+		cdr->is_root = 1;
 		ao2_link(active_cdrs_by_channel, cdr);
 	}
 

-- 
To view, visit https://gerrit.asterisk.org/4151
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3ca90c2b8051f3b7ead2e0e43f60d2c18fb204b8
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>