[Spice-devel] [PATCH v2] gstreamer: Fix infinite loop in get_period_bit_rate

[Frediano Ziglio]

This was discovered by chance by me and Uri trying some remote connection
with slow network (8Mbit) and high latency

  $ ping 10.10.48.87 -c 3
  3 packets transmitted, 3 received, 0% packet loss, time 2002ms
  rtt min/avg/max/mdev = 281.069/316.758/374.413/41.153 ms

The encoder->history status was (edited for readability):

(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_first
$6 = 29
(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_last
$14 = 28
(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history
[0] {mm_time = 11298131, duration = 7391006, size = 5616},
[1] {mm_time = 11298148, duration = 7373663, size = 5559},
[2] {mm_time = 11298166, duration = 7052209, size = 5511},
[3] {mm_time = 11298183, duration = 7006828, size = 5722},
[4] {mm_time = 11298199, duration = 7433311, size = 5756},
[5] {mm_time = 11298216, duration = 7134734, size = 5545},
[6] {mm_time = 11298232, duration = 7436589, size = 5521},
[7] {mm_time = 11298249, duration = 7152181, size = 5540},
[8] {mm_time = 11298266, duration = 7181308, size = 7796},
[9] {mm_time = 11298283, duration = 5053084, size = 50824},
[10] {mm_time = 11298298, duration = 7472305, size = 7427},
[11] {mm_time = 11298315, duration = 7385017, size = 6682},
[12] {mm_time = 11298333, duration = 7287125, size = 6377},
[13] {mm_time = 11298349, duration = 7191461, size = 6159},
[14] {mm_time = 11298367, duration = 7104546, size = 6035},
[15] {mm_time = 11298382, duration = 7266942, size = 5896},
[16] {mm_time = 11298400, duration = 7108001, size = 5812},
[17] {mm_time = 11298418, duration = 7020583, size = 5807},
[18] {mm_time = 11298433, duration = 7066056, size = 5758},
[19] {mm_time = 11298450, duration = 7052900, size = 5676},
[20] {mm_time = 11298467, duration = 7248233, size = 5765},
[21] {mm_time = 11298483, duration = 7077348, size = 5712},
[22] {mm_time = 11298502, duration = 7495368, size = 5835},
[23] {mm_time = 11298517, duration = 7068626, size = 5805},
[24] {mm_time = 11298534, duration = 7060375, size = 5801},
[25] {mm_time = 11298551, duration = 7020383, size = 5868},
[26] {mm_time = 11298568, duration = 7248400, size = 5830},
[27] {mm_time = 11298584, duration = 7001304, size = 6621},
[28] {mm_time = 11298600, duration = 7311600, size = 6113},
[29] {mm_time = 11297612, duration = 6999174, size = 5666},  <--- to
[30] {mm_time = 11297628, duration = 7506688, size = 5502},
[31] {mm_time = 11297646, duration = 7209494, size = 5687},
[32] {mm_time = 11297663, duration = 7396429, size = 5724},
[33] {mm_time = 11297679, duration = 7172624, size = 5839},
[34] {mm_time = 11297696, duration = 7300811, size = 5645},
[35] {mm_time = 11297713, duration = 7108985, size = 5553},
[36] {mm_time = 11297729, duration = 7171701, size = 5774},
[37] {mm_time = 11297745, duration = 7174018, size = 5637},
[38] {mm_time = 11297762, duration = 7313549, size = 5655},
[39] {mm_time = 11297780, duration = 5183985, size = 51014},
[40] {mm_time = 11297796, duration = 7038329, size = 7374},
[41] {mm_time = 11297813, duration = 7211506, size = 6585},
[42] {mm_time = 11297830, duration = 7112690, size = 5729},
[43] {mm_time = 11297846, duration = 7103074, size = 5761},
[44] {mm_time = 11297864, duration = 7599826, size = 5661},
[45] {mm_time = 11297879, duration = 7355392, size = 5351},
[46] {mm_time = 11297897, duration = 7454367, size = 5488},
[47] {mm_time = 11297913, duration = 7127145, size = 5573},
[48] {mm_time = 11297932, duration = 7550098, size = 5447},
[49] {mm_time = 11297948, duration = 7506884, size = 5809},
[50] {mm_time = 11297966, duration = 7405712, size = 5783},
[51] {mm_time = 11297982, duration = 7182025, size = 5599},
[52] {mm_time = 11298001, duration = 7323887, size = 5817},
[53] {mm_time = 11298014, duration = 7342091, size = 5575},
[54] {mm_time = 11298031, duration = 7596319, size = 5739},
[55] {mm_time = 11298052, duration = 7428169, size = 5669},
[56] {mm_time = 11298065, duration = 7457282, size = 5732},
[57] {mm_time = 11298081, duration = 7174029, size = 5541},
[58] {mm_time = 11298097, duration = 7340512, size = 5680},
[59] {mm_time = 11298115, duration = 7427439, size = 5701}}
(gdb) p from
$16 = 11297544
(gdb) p to
$17 = 11297612

You can see that encoder->history[encoder->history_first].mm_time == to
cause the check index == encoder->history_first to not be executed.

This code change was suggested by Uri.

Reported-by: Uri Lublin <uril at redhat.com>
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Tested-by: Uri Lublin <uril at redhat.com>
---
 server/gstreamer-encoder.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Changes since v1:
- add an empty line in code;
- commit message typo.

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5a4b7fd..c1063ea 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -434,19 +434,20 @@ static uint64_t get_period_bit_rate(SpiceGstEncoder *encoder, uint32_t from,
             sum += encoder->history[index].size;
             return (sum - 1) * 8 * MSEC_PER_SEC / (last_mm_time - from);
 
-        } else if (index == encoder->history_first) {
+        } else if (sum > 0) {
+            sum += encoder->history[index].size;
+
+        } else {
+            last_mm_time = encoder->history[index].mm_time;
+        }
+
+        if (index == encoder->history_first) {
             /* This period is outside the recorded history */
             spice_debug("period (%u-%u) outside known history (%u-%u)",
                         from, to,
                         encoder->history[encoder->history_first].mm_time,
                         encoder->history[encoder->history_last].mm_time);
            return 0;
-
-        } else if (sum > 0) {
-            sum += encoder->history[index].size;
-
-        } else {
-            last_mm_time = encoder->history[index].mm_time;
         }
         index = (index ? index : SPICE_GST_HISTORY_SIZE) - 1;
     }
-- 
2.7.4