GNOME Bugzilla – Bug 755971
queue2: may overflow unsigned integer arithmetic
Last modified: 2016-01-11 22:04:16 UTC
Queue2's GET_PERCENT macro multiplies cur_level by 100: #define GET_PERCENT(format,alt_max) ((queue->max_level.format) > 0 ? (queue->cur_level.format) * 100 / ((alt_max) > 0 ? [...] If this overflows, we may get negative percentage, which will be then stored as signed integer, and later used for gst_message_new_buffering(), that will validate if percent is between 0 and 100, and return NULL (or abort, if G_DEBUG=fatal-warnings). The problem was reproduced when queueing invalid data in an artificial testcase (segment started in 0, but the buffer passed to queue2 had timestamp 18446744071709551616. Still, queue2 should not cause crashes on such data.
Created attachment 312551 [details] [review] The testcase Attaching the testcase. When running make check, then the queue2 test will fail, and the log contains the following info: Running suite(s): queue2 Unexpected critical/warning: gst_message_new_buffering: assertion 'percent >= 0 && percent <= 100' failed 85%: Checks: 7, Failures: 1, Errors: 0 gstcheck.c:79:F:general:test_percent_overflow:0: Unexpected critical/warning: gst_message_new_buffering: assertion 'percent >= 0 && percent <= 100' failed FAIL elements/queue2 (exit status: 1)
Do you want to provide a patch? Using gst_util_uint64_scale() might be an option here.
I will provide a patch, though I'm not working on it now. If anyone else decides that it's important enough that he or she creates a patch first, I'd be more than happy ;) If not, maybe in a week or two I'll find time (looks like a simple issue, should not take much time once I start it).
Created attachment 312709 [details] [review] The fix Attaching the patch. It does 2 things: - uses gst_util_uint64_scale() so the percent calculation does not overflow (on guint64 type) - uses MIN (100, gst_util_uint64_scale(...)) to ensure that the result also does not overflow integer. It does not remove "if (perc > 100) perc = 100;" check, as that one is done after scaling the percent to high_percent, so it's actually needed.
Review of attachment 312551 [details] [review]: I think this test just need a small portability change, but seems very good otherwise. ::: tests/check/elements/queue2.c @@ +314,3 @@ + for (i = 0; i < 20; i++) { + buffer = gst_buffer_new_and_alloc (1024); + GST_BUFFER_PTS (buffer) = 18446744071709551616ULL + i * (GST_SECOND / 10); /* 10 FPS */ Use G_GUINT64_CONSTANT() macro for portability.
Created attachment 312723 [details] [review] Review issues fixed in the unit test Unit test fixed.
Review of attachment 312709 [details] [review]: ::: plugins/elements/gstqueue2.c @@ +836,3 @@ return FALSE; } +#define GET_PERCENT(format,alt_max) ((queue->max_level.format) > 0 ? MIN (100, gst_util_uint64_scale(queue->cur_level.format, 100, ((alt_max) > 0 ? MIN ((alt_max), (queue->max_level.format)) : (queue->max_level.format)))) : 0) This is way over 80 char limit by the coding standard (the thing to fix absolutly). I would allow myself to suggest to make this an inline function, with proper if statement. This will be less likely prone to hide errors for the future.
Created attachment 312724 [details] [review] The testcase One more update to the testcase (formatting).
Review of attachment 312723 [details] [review]: The change ended exceeding 80 chars. I took this opportunity to note other small things. ::: tests/check/elements/queue2.c @@ +299,3 @@ + "max-size-buffers", (guint) 0, + "max-size-time", (guint64) 2 * GST_SECOND, + "max-size-bytes", (guint) 0, NULL); All 3 casts are useless here. 2 * GST_SECOND will do the multiplation in guint64, for the rest guint and gint are binary compatible for vararg. @@ +314,3 @@ + for (i = 0; i < 20; i++) { + buffer = gst_buffer_new_and_alloc (1024); + GST_BUFFER_PTS (buffer) = G_GUINT64_CONSTANT(18446744071709551616) + i * (GST_SECOND / 10); /* 10 FPS */ That exceeds 80 chat.
Created attachment 312725 [details] [review] The testcase The testcase now passing pre-commit hooks.
> That exceeds 80 char. Uh, that one was not caught by the pre-commit hook. I'll correct it separately, as well as the casts.
Created attachment 312726 [details] [review] The fix I've split the macro into multiple lines, aligning parts of expressions by indentation. Should be easy to read (as much as the nested ternary operators can be...) Making it an inline function would need some refactoring, the macro takes as an argument the identifier of the structure field, can't pass it to inline.
Created attachment 312728 [details] [review] The testcase Fixed line length.
Review of attachment 312726 [details] [review]: ::: plugins/elements/gstqueue2.c @@ +837,3 @@ } +#define GET_PERCENT(format,alt_max) \ + queue->max_level.format > 0 \ You need to add an extra parentheses around macros, around all use of the input parameters. You'll have to store format into a variable as you expend it multiple time, otherwise you may have side effects. In the end, I'd say just change that into an inline function.
commit 90318900a192e341833db5d05c33a39c8bc9b210 Author: Aleksander Wabik <awabik@opera.com> Date: Tue Oct 6 12:49:00 2015 +0000 tests: queue2: add test for fill level arithmetic overflow https://bugzilla.gnome.org/show_bug.cgi?id=755971 commit 61e2f1eab1601b963b220f4d33ab869c9752eabc Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Jan 6 19:51:44 2016 +0000 queue2: avoid calculating fill levels multiple times Macro expansion means we might calculate the fill level once for the check and then possibly again for the return value. commit 8c6c0bef884197808745e0fc9e9aa814b826a437 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Jan 6 19:50:21 2016 +0000 queue2: fix fill level arithmetic overflow with large values Based on patch by: Aleksander Wabik <awabik@opera.com> https://bugzilla.gnome.org/show_bug.cgi?id=755971