[Spice-devel] [spice-gtk v2 1/2] Fix possible multimedia time overflow

[Frediano Ziglio]

The multimedia time can easily overflow as is encoded in an
unsigned 32 bit and have a unit of milliseconds so it wrap
up every 49 days. Use some math that allow the number to overflow
without issues.
This could caused the client to stop handling streaming and
starting only queueing.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
This patch should be applied independently on 2/2 as intended to be
merged upstream as a fix while 2/2 depends on this as it change same
code portions.
---
 src/channel-display-gst.c   | 11 ++++++-----
 src/channel-display-mjpeg.c | 14 ++++++++------
 src/channel-display-priv.h  | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c4190b2..9c62e67 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -183,8 +183,9 @@ static void schedule_frame(SpiceGstDecoder *decoder)
         }
 
         SpiceStreamDataHeader *op = spice_msg_in_parsed(frame->msg);
-        if (now < op->multi_media_time) {
-            decoder->timer_id = g_timeout_add(op->multi_media_time - now,
+        gint32 time_diff = compute_mm_time_diff(op->multi_media_time, now);
+        if (time_diff >= 0) {
+            decoder->timer_id = g_timeout_add(time_diff,
                                               display_frame, decoder);
         } else if (g_queue_get_length(decoder->display_queue) == 1) {
             /* Still attempt to display the least out of date frame so the
@@ -192,8 +193,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
              */
             decoder->timer_id = g_timeout_add(0, display_frame, decoder);
         } else {
-            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime: %u), dropping",
-                        __FUNCTION__, now - op->multi_media_time,
+            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime: %u), dropping",
+                        __FUNCTION__, -time_diff,
                         op->multi_media_time, now);
             stream_dropped_frame_on_playback(decoder->base.stream);
             g_queue_pop_head(decoder->display_queue);
@@ -431,7 +432,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
     }
 
     SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
-    if (frame_op->multi_media_time < decoder->last_mm_time) {
+    if (compute_mm_time_diff(frame_op->multi_media_time, decoder->last_mm_time) < 0) {
         SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
                     " resetting stream, id %u",
                     frame_op->multi_media_time,
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 722494e..1b7373b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -195,15 +195,15 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
     do {
         if (frame_msg) {
             SpiceStreamDataHeader *op = spice_msg_in_parsed(frame_msg);
-            if (time <= op->multi_media_time) {
-                guint32 d = op->multi_media_time - time;
+            gint32 time_diff = compute_mm_time_diff(op->multi_media_time, time);
+            if (time_diff >= 0) {
                 decoder->cur_frame_msg = frame_msg;
-                decoder->timer_id = g_timeout_add(d, mjpeg_decoder_decode_frame, decoder);
+                decoder->timer_id = g_timeout_add(time_diff, mjpeg_decoder_decode_frame, decoder);
                 break;
             }
 
-            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime: %u), dropping ",
-                        __FUNCTION__, time - op->multi_media_time,
+            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime: %u), dropping ",
+                        __FUNCTION__, -time_diff,
                         op->multi_media_time, time);
             stream_dropped_frame_on_playback(decoder->base.stream);
             spice_msg_in_unref(frame_msg);
@@ -249,7 +249,9 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
         SpiceStreamDataHeader *last_op, *frame_op;
         last_op = spice_msg_in_parsed(last_msg);
         frame_op = spice_msg_in_parsed(frame_msg);
-        if (frame_op->multi_media_time < last_op->multi_media_time) {
+        gint32 time_diff =
+            compute_mm_time_diff(frame_op->multi_media_time, last_op->multi_media_time);
+        if (time_diff < 0) {
             /* This should really not happen */
             SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
                         " resetting stream, id %u",
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index b9c08a3..eced6af 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -141,6 +141,25 @@ void stream_dropped_frame_on_playback(display_stream *st);
 void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint32_t width, uint32_t height, uint8_t *data);
 uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data);
 
+/* Compute the difference between 2 multimedia times.
+ * Multimedia times are subject to overflow so the check
+ * for time1 < time2 does not always indicate that time2
+ * happens after time1.
+ * So define a function to compute the difference and
+ * use as documentation for the code.
+ */
+static inline gint32 compute_mm_time_diff(guint32 time1, guint32 time2)
+{
+    /* Compute time difference returning a signed value
+     * For instance:
+     * - if time1 == 3 and time2 == UINT32_MAX this means that
+     *   time1 overflowed and is 4 milliseconds after time2;
+     * - if time1 == UINT32_MAX and time2 == 3 this means that
+     *   time2 overflowed and is 4 milliseconds before time2
+     *   (so the difference is -4)
+     */
+    return (gint32) (time1 - time2);
+}
 
 G_END_DECLS
 
-- 
2.9.3