[Asterisk-code-review] jitterbuffer: Correct signed/unsigned mismatch causing assert (asterisk[master])

[Friendly Automation]

Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/16111 )

Change subject: jitterbuffer:  Correct signed/unsigned mismatch causing assert
......................................................................

jitterbuffer:  Correct signed/unsigned mismatch causing assert

If the system time has stepped backwards because of a time
adjustment between the time a frame is timestamped and the
time we check the timestamps in abstract_jb:hook_event_cb(),
we get a negative interval, but we don't check for that there.
abstract_jb:hook_event_cb() then calls
fixedjitterbuffer:fixed_jb_get() (via abstract_jb:jb_get_fixed)
and the first thing that does is assert(interval >= 0).

There are several issues with this...

 * abstract_jb:hook_event_cb() saves the interval in a variable
   named "now" which is confusing in itself.

 * "now" is defined as an unsigned int which converts the negative
   value returned from ast_tvdiff_ms() to a large positive value.

 * fixed_jb_get()'s parameter is defined as a signed int so the
   interval gets converted back to a negative value.

 * fixed_jb_get()'s assert is NOT an ast_assert but a direct define
   that points to the system assert() so it triggers even in
   production mode.

So...

 * hook_event_cb()'s "now" was renamed to "relative_frame_start" and
   changed to an int64_t.
 * hook_event_cb() now checks for a negative value right after
   retrieving both the current and framedata timestamps and just
   returns the frame if the difference is negative.
 * fixed_jb_get()'s local define of ASSERT() was changed to call
   ast_assert() instead of the system assert().

ASTERISK-29480
Reported by: Dan Cropp

Change-Id: Ic469dec73c2edc3ba134cda6721a999a9714f3c9
---
M main/abstract_jb.c
M main/fixedjitterbuf.c
2 files changed, 19 insertions(+), 9 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Friendly Automation: Approved for Submit



diff --git a/main/abstract_jb.c b/main/abstract_jb.c
index 7549008..083ff9e 100644
--- a/main/abstract_jb.c
+++ b/main/abstract_jb.c
@@ -954,7 +954,7 @@
 {
 	struct jb_framedata *framedata = data;
 	struct timeval now_tv;
-	unsigned long now;
+	int64_t relative_frame_start;
 	int putframe = 0; /* signifies if audio frame was placed into the buffer or not */
 
 	switch (event) {
@@ -1064,7 +1064,17 @@
 	}
 
 	now_tv = ast_tvnow();
-	now = ast_tvdiff_ms(now_tv, framedata->start_tv);
+	relative_frame_start = ast_tvdiff_ms(now_tv, framedata->start_tv);
+	if (relative_frame_start < 0) {
+		/*
+		 * The only way for this to happen is if the system time has
+		 * stepped backwards between the time framedata->start_tv was
+		 * set and now.  Think an ntpd or systemd-timesyncd adjustment.
+		 *
+		 * Just pass the frame through.
+		 */
+		return frame;
+	}
 
 	if (frame->frametype == AST_FRAME_VOICE) {
 		int res;
@@ -1084,9 +1094,9 @@
 		}
 		if (!framedata->first) {
 			framedata->first = 1;
-			res = framedata->jb_impl->put_first(framedata->jb_obj, jbframe, now);
+			res = framedata->jb_impl->put_first(framedata->jb_obj, jbframe, relative_frame_start);
 		} else {
-			res = framedata->jb_impl->put(framedata->jb_obj, jbframe, now);
+			res = framedata->jb_impl->put(framedata->jb_obj, jbframe, relative_frame_start);
 		}
 
 		if (res == AST_JB_IMPL_OK) {
@@ -1104,7 +1114,7 @@
 		int res;
 		long next = framedata->jb_impl->next(framedata->jb_obj);
 
-		/* If now is earlier than the next expected output frame
+		/* If relative_frame_start is earlier than the next expected output frame
 		 * from the jitterbuffer we may choose to pass on retrieving
 		 * a frame during this read iteration.  The only exception
 		 * to this rule is when an audio frame is placed into the buffer
@@ -1113,8 +1123,8 @@
 		 * doing this we are able to feed off the timing of the input frames
 		 * and only rely on our jitterbuffer timer when frames are dropped.
 		 * During testing, this hybrid form of timing gave more reliable results. */
-		if (now < next) {
-			long int diff = next - now;
+		if (relative_frame_start < next) {
+			long int diff = next - relative_frame_start;
 			if (!putframe) {
 				return frame;
 			} else if (diff >= framedata->timer_interval) {
@@ -1124,7 +1134,7 @@
 
 		ast_frfree(frame);
 		frame = &ast_null_frame;
-		res = framedata->jb_impl->get(framedata->jb_obj, &frame, now, framedata->timer_interval);
+		res = framedata->jb_impl->get(framedata->jb_obj, &frame, relative_frame_start, framedata->timer_interval);
 		switch (res) {
 		case AST_JB_IMPL_OK:
 			/* got it, and pass it through */
diff --git a/main/fixedjitterbuf.c b/main/fixedjitterbuf.c
index 3c350b4..6c92329 100644
--- a/main/fixedjitterbuf.c
+++ b/main/fixedjitterbuf.c
@@ -41,7 +41,7 @@
 #ifdef FIXED_JB_DEBUG
 #define ASSERT(a)
 #else
-#define ASSERT(a) assert(a)
+#define ASSERT(a) ast_assert(a)
 #endif
 
 /*! \brief private fixed_jb structure */

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ic469dec73c2edc3ba134cda6721a999a9714f3c9
Gerrit-Change-Number: 16111
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at sangoma.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph 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/20210624/da20fbd7/attachment.html>