From 88da59efe7067de216d24eb74141c1c3a23f992f Mon Sep 17 00:00:00 2001
From: George Joseph <gjoseph@digium.com>
Date: Thu, 17 Jun 2021 06:57:11 -0600
Subject: [PATCH] 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
---
main/abstract_jb.c | 26 ++++++++++++++++++--------
main/fixedjitterbuf.c | 2 +-
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/main/abstract_jb.c b/main/abstract_jb.c
index 7549008cda..083ff9efb6 100644
--- a/main/abstract_jb.c
+++ b/main/abstract_jb.c
@@ -954,7 +954,7 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
{
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 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
}
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 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
}
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 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
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 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
* 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 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
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 3c350b4bc4..6c923296c7 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 */
--
GitLab