[svn-commits] mmichelson: branch 1.4 r269821 - /branches/1.4/main/channel.c

[SVN commits to the Digium repositories]

Author: mmichelson
Date: Thu Jun 10 14:30:12 2010
New Revision: 269821

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=269821
Log:
Fix potential crash when writing raw SLIN audio on a PLC-enabled channel.

The issue here was that the frame created when adjusting for PLC had no offset
to its audio data. If this frame were translated to another format prior to
being sent out an RTP socket, all went well because the translation code would
put an appropriate offset into the frame. However, if the SLIN audio were not
translated before being sent out the RTP socket, bad things would happen.
Specifically, the ast_rtp_raw_write makes the assumption that the frame has
at least enough of an offset that it can accommodate an RTP header. This was
not the case. As such, data was being written prior to the allocation, likely
corrupting the data the memory allocator had written. Thus when the time came
to free the data, all hell broke loose. ....Well, Asterisk crashed at least.

The fix was just what one would expect. Offset the data in the frame by a reasonable
amount. The method I used is a bit odd since the data in the frame is 16 bit integers
and not bytes. I left a big ol' comment about it. This can be improved on if someone
is interested. I was more interested in getting the crash resolved.


Modified:
    branches/1.4/main/channel.c

Modified: branches/1.4/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/main/channel.c?view=diff&rev=269821&r1=269820&r2=269821
==============================================================================
--- branches/1.4/main/channel.c (original)
+++ branches/1.4/main/channel.c Thu Jun 10 14:30:12 2010
@@ -2938,6 +2938,24 @@
 	int num_new_samples = frame->samples;
 	struct plc_ds *plc = datastore->data;
 
+	/* As a general note, let me explain the somewhat odd calculations used when taking
+	 * the frame offset into account here. According to documentation in frame.h, the frame's
+	 * offset field indicates the number of bytes that the audio is offset. The plc->samples_buf
+	 * is not an array of bytes, but rather an array of 16-bit integers since it holds SLIN
+	 * samples. So I had two choices to make here with the offset.
+	 * 
+	 * 1. Make the offset AST_FRIENDLY_OFFSET bytes. The main downside for this is that
+	 *    I can't just add AST_FRIENDLY_OFFSET to the plc->samples_buf and have the pointer
+	 *    arithmetic come out right. I would have to do some odd casting or division for this to
+	 *    work as I wanted.
+	 * 2. Make the offset AST_FRIENDLY_OFFSET * 2 bytes. This allows the pointer arithmetic
+	 *    to work out better with the plc->samples_buf. The downside here is that the buffer's
+	 *    allocation contains an extra 64 bytes of unused space.
+	 * 
+	 * I decided to go with option 2. This is why in the calloc statement and the statement that
+	 * sets the frame's offset, AST_FRIENDLY_OFFSET is multiplied by 2.
+	 */
+
 	/* If this audio frame has no samples to fill in, ignore it */
 	if (!num_new_samples) {
 		return;
@@ -2949,7 +2967,7 @@
 	 */
 	if (plc->num_samples < num_new_samples) {
 		ast_free(plc->samples_buf);
-		plc->samples_buf = ast_calloc(num_new_samples, sizeof(*plc->samples_buf));
+		plc->samples_buf = ast_calloc(1, (num_new_samples * sizeof(*plc->samples_buf)) + (AST_FRIENDLY_OFFSET * 2));
 		if (!plc->samples_buf) {
 			ast_channel_datastore_remove(chan, datastore);
 			ast_channel_datastore_free(datastore);
@@ -2959,9 +2977,10 @@
 	}
 
 	if (frame->datalen == 0) {
-		plc_fillin(&plc->plc_state, plc->samples_buf, frame->samples);
-		frame->data = plc->samples_buf;
+		plc_fillin(&plc->plc_state, plc->samples_buf + AST_FRIENDLY_OFFSET, frame->samples);
+		frame->data = plc->samples_buf + AST_FRIENDLY_OFFSET;
 		frame->datalen = num_new_samples * 2;
+		frame->offset = AST_FRIENDLY_OFFSET * 2;
 	} else {
 		plc_rx(&plc->plc_state, frame->data, frame->samples);
 	}