[asterisk-dev] iax2-parser.c - Memory

[Russell Bryant]

Octavio Ascanio Su‡árez wrote:
> I am Octavio, I am working with Eric in this problem. We have added some 
> ast_log in iax_frame_free and iax_frame_new we have seen that 
> iax_frame_new dont find a block of 160 bytes in the linked list when 
> iax_frame_free has added it previously.

Keep in mind that just because you saw a frame of that size go into 
iax_frame_free, does not mean that is available for the next call to 
iax_frame_new.  There is a cache of available frames _per thread_.  So, 
if the call to iax_frame_free is done from a different thread than the 
call to iax_frame_new, then this behavior would be expected.

It is possible that there is a thread (or number of a specific type of 
threads) in chan_iax2 which has code that frees frames, but does not 
allocate them.  If that is the case, then the frame cache for that 
thread would grow for forever.

If that is the issue we're facing, then the following patch would avoid 
the infinite leak.  However, a full solution would be to add an argument 
for iax_frame_free which hints that the frame shouldn't be cached when 
we know it will never be pulled out of the cache.

Index: channels/iax2-parser.c
===================================================================
--- channels/iax2-parser.c	(revision 133577)
+++ channels/iax2-parser.c	(working copy)
@@ -58,7 +58,14 @@

  /*! \brief This is just so iax_frames, a list head struct for holding 
a list of
   *  iax_frame structures, is defined. */
-AST_LIST_HEAD_NOLOCK(iax_frames, iax_frame);
+AST_LIST_HEAD_NOLOCK(iax_frame_list, iax_frame);
+
+struct iax_frames {
+	struct iax_frame_list list;
+	size_t size;
+};
+
+#define FRAME_CACHE_MAX_SIZE	20
  #endif

  static void internaloutput(const char *str)
@@ -957,10 +964,11 @@

  	/* Attempt to get a frame from this thread's cache */
  	if ((iax_frames = ast_threadstorage_get(&frame_cache, 
sizeof(*iax_frames)))) {
-		AST_LIST_TRAVERSE_SAFE_BEGIN(iax_frames, fr, list) {
+		AST_LIST_TRAVERSE_SAFE_BEGIN(&iax_frames->list, fr, list) {
  			if (fr->afdatalen >= datalen) {
  				size_t afdatalen = fr->afdatalen;
-				AST_LIST_REMOVE_CURRENT(iax_frames, list);
+				AST_LIST_REMOVE_CURRENT(&iax_frames->list, list);
+				iax_frames->size--;
  				memset(fr, 0, sizeof(*fr));
  				fr->afdatalen = afdatalen;
  				break;
@@ -1017,11 +1025,14 @@
  		return;
  	}

-	fr->direction = 0;
-	AST_LIST_INSERT_HEAD(iax_frames, fr, list);
-#else
+	if (iax_frames->size < FRAME_CACHE_MAX_SIZE) {
+		fr->direction = 0;
+		AST_LIST_INSERT_HEAD(&iax_frames->list, fr, list);
+		iax_frames->size++;
+		return;
+	}
+#endif
  	free(fr);
-#endif
  }

  #if !defined(LOW_MEMORY)
@@ -1030,7 +1041,7 @@
  	struct iax_frames *frames = data;
  	struct iax_frame *cur;

-	while ((cur = AST_LIST_REMOVE_HEAD(frames, list)))
+	while ((cur = AST_LIST_REMOVE_HEAD(&frames->list, list)))
  		free(cur);

  	free(frames);


-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.