After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752336 - dashdemux: duration field could overflow
dashdemux: duration field could overflow
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 752337 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-07-13 15:53 UTC by Florin Apostol
Modified: 2015-11-02 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
validation on duration specification (5.35 KB, patch)
2015-09-17 14:13 UTC, Vincent Penquerc'h
none Details | Review
validation on duration specification (5.00 KB, patch)
2015-09-17 15:10 UTC, Vincent Penquerc'h
none Details | Review
proposed patch for duration format validation (4.34 KB, patch)
2015-09-17 16:11 UTC, Florin Apostol
none Details | Review
validation on duration specification (12.02 KB, patch)
2015-09-29 09:45 UTC, Vincent Penquerc'h
none Details | Review
unit test for duration parsing (4.19 KB, patch)
2015-09-29 09:46 UTC, Vincent Penquerc'h
none Details | Review
1/5 (2.73 KB, patch)
2015-10-01 14:30 UTC, Vincent Penquerc'h
none Details | Review
2/5 (22.50 KB, patch)
2015-10-01 14:31 UTC, Vincent Penquerc'h
none Details | Review
3/5 (4.19 KB, patch)
2015-10-01 14:31 UTC, Vincent Penquerc'h
committed Details | Review
4/5 (12.02 KB, patch)
2015-10-01 14:31 UTC, Vincent Penquerc'h
none Details | Review
5/5 (3.62 KB, patch)
2015-10-01 14:32 UTC, Vincent Penquerc'h
none Details | Review
4/5 - add checks to duration parsing (12.33 KB, application/mbox)
2015-10-01 15:12 UTC, Vincent Penquerc'h
  Details
5/5 - unit test (4.28 KB, patch)
2015-10-01 15:12 UTC, Vincent Penquerc'h
none Details | Review
4/5 - add checks to duration parsing (12.33 KB, patch)
2015-10-01 15:37 UTC, Vincent Penquerc'h
none Details | Review
4/5 - add checks to duration parsing (12.35 KB, patch)
2015-10-01 16:21 UTC, Vincent Penquerc'h
none Details | Review
4/5 - add checks to duration parsing (11.87 KB, patch)
2015-11-02 11:08 UTC, Vincent Penquerc'h
committed Details | Review
5/5 - unit test (4.47 KB, patch)
2015-11-02 11:08 UTC, Vincent Penquerc'h
committed Details | Review

Description Florin Apostol 2015-07-13 15:53:23 UTC
The gst_mpdparser_get_xml_prop_duration function reads a duration from xml file, converts this to ms and returns it into a gint64 variable. The value will later be usually converted to nanoseconds and stored into a GstClockTime (which is also 64 bits).

Durations longer than 584 years will overflow the GstClockTime datatype. For example, a duration of P584Y will be 0xFF9669C01BD80000 nanoseconds, but a duration of P585Y will be 0x00067393497B0000 nanoseconds.

The gst_mpdparser_get_xml_prop_duration function has the following algorithm for converting the read values into a ms value:

    *property_value =
        sign * ((((((gint64) years * 365 + months * 30 + days) * 24 +
                    hours) * 60 + minutes) * 60 + seconds) * 1000 + decimals);

This should be enhanced to detect overflows, and more, future overflows when this will be converted to a nanosecond value. Basically, the function should return an error if the duration is bigger than 584 (and something) years.
Comment 1 Vincent Penquerc'h 2015-09-17 14:13:45 UTC
Created attachment 311556 [details] [review]
validation on duration specification
Comment 2 Florin Apostol 2015-09-17 14:43:54 UTC
Review of attachment 311556 [details] [review]:

This is so complicated! Just to check if the month is set before the year!
Why not initialize year, month, day to 0 and before assigning year=read you check if the month or day was set! Easy and clear, you add just 2 ifs, one for year, one for month. Similar for time.

::: ext/dash/gstmpdparser.c
@@ +917,2 @@
             years = read;
+            if (years > 584) {  /* see https://bugzilla.gnome.org/show_bug.cgi?id=752336 for rationale */

I don't think we need to limit it like this. We should detect a possible overflow during the calculation of property_value

@@ +961,3 @@
     pos = 0;
     if (pos < len) {
+      /* T units_found, there is a time section */

why did you changed this? Possible a find and replace error
Comment 3 Vincent Penquerc'h 2015-09-17 15:07:09 UTC
(In reply to Florin Apostol from comment #2)
> +      /* T units_found, there is a time section */
> 
> why did you changed this? Possible a find and replace error

Yes, it is, sorry.
Comment 4 Vincent Penquerc'h 2015-09-17 15:10:15 UTC
Created attachment 311565 [details] [review]
validation on duration specification

With fixed stray replace.

For the validation code, I'll probably be fine with whatever code you prefer.
Comment 5 Vincent Penquerc'h 2015-09-17 15:16:58 UTC
0 is a valid value AFAICT. If you initialized to -1 or other sentinel value instead, you'd have to test for that before accumulating at the end. If only test two values, you false negative on something like H1Y1, no ?

But, again, if you're happy with something else, fine too.
Comment 6 Vincent Penquerc'h 2015-09-17 15:18:23 UTC
Or 1H1Y since the types are postfix.
Comment 7 Florin Apostol 2015-09-17 16:11:02 UTC
Created attachment 311567 [details] [review]
proposed patch for duration format validation

I was thinking about something like this for duration format validation.
It needs checks for months >=31, hours >=24, etc
Also the function should be moved to a separate function that extracts duration from a string so that we can add some simple unit tests.
Comment 8 Vincent Penquerc'h 2015-09-17 16:21:20 UTC
This will let through something like H1Y1 still. Unless maybe there is only partlal ordering being mandated (date ordered within itself, HMS ordered between itself, but it's fine to have date-hms or hms-date) ?

Otherwise, looks ok with the added range checks you mention.
Comment 9 Florin Apostol 2015-09-17 16:29:02 UTC
(In reply to Vincent Penquerc'h from comment #8)
> This will let through something like H1Y1 still. Unless maybe there is only
> partlal ordering being mandated (date ordered within itself, HMS ordered
> between itself, but it's fine to have date-hms or hms-date) ?
> 
No, the order is important. More, it is separated by T.
It will not accept P1H1Y. It will search for P. From there, it will search for number + character. If the character is not YMD, it will fail.

Let's move the code into a separate function that receives a string and let's add a unit test to test all the scenarios you can think of!
Comment 10 Vincent Penquerc'h 2015-09-17 16:33:30 UTC
Oh right, I'd missed that YMD and HMS are in two separate loops. This had confused me earlier. I see now why you don't need that many checks for order.
Comment 11 Vincent Penquerc'h 2015-09-29 09:45:37 UTC
Created attachment 312351 [details] [review]
validation on duration specification

To be applied on top of your patch.
Comment 12 Vincent Penquerc'h 2015-09-29 09:46:03 UTC
Created attachment 312352 [details] [review]
unit test for duration parsing
Comment 13 Florin Apostol 2015-09-30 15:44:13 UTC
(In reply to Vincent Penquerc'h from comment #11)
> Created attachment 312351 [details] [review] [review]
> validation on duration specification
> 
> To be applied on top of your patch.

I can't apply this on top of my patch or directly on master. It seems you did not provide the complete set of patches.
Comment 14 Vincent Penquerc'h 2015-09-30 15:55:54 UTC
Indeed. I put the four relevant patches on top of master here: https://git.collabora.com/cgit/user/vincent/gst-plugins-bad/log/?h=dash-duration

The patch in this bug builds upon other patches dealing with making duration unsigned.
Comment 15 Florin Apostol 2015-10-01 11:18:12 UTC
(In reply to Vincent Penquerc'h from comment #14)
> Indeed. I put the four relevant patches on top of master here:
> https://git.collabora.com/cgit/user/vincent/gst-plugins-bad/log/?h=dash-
> duration
> 
> The patch in this bug builds upon other patches dealing with making duration
> unsigned.

That tree is temporary and you can change it at any time. If other people will want to review later these patches, they will not be able to apply them. Also, after review, when the maintainer will try to apply them he will not be able to do that without bringing in other patches, which maybe are not yet reviewed and accepted.
I suggest you attach to a ticket the whole set of changes you propose for that ticket (even if some patches are duplicated in other tickets). In this way everybody can apply the patches on master and see the resulting code.
Comment 16 Florin Apostol 2015-10-01 12:15:37 UTC
Review of attachment 312351 [details] [review]:

::: ext/dash/gstmpdparser.c
@@ +853,1 @@
+  if (strspn (str, "PT0123456789., HMDSY") < strlen (str)) {

maybe /t is also valid. I don't know.
And maybe we should allow spaces only at the begging and the end of the string, not also in the middle.

@@ +853,2 @@
+  if (strspn (str, "PT0123456789., HMDSY") < strlen (str)) {
+    GST_WARNING ("Invalid character found: %s", str);

put %s in quotes: '%s'. In this way you will know when a space is reported.

@@ +858,1 @@
+  len = strlen (str);

move this above, so that you don't call strlen twice

@@ +858,2 @@
+  len = strlen (str);
+  GST_TRACE ("duration: %s, len %d", str, len);

move this at the beginning of the function, before strspn can detect errors.

@@ +860,3 @@
+  /* read "P" for period */
+  pos = strcspn (str, "P");
+  if (pos != 0) {

why not simply check str[0] ?
And we should skip spaces first.

@@ +870,3 @@
+  len -= posT;
+  if (posT > 0) {
+    /* there is some room between P and T, so there must be a period section */

unless there are only spaces. Should we allow spaces in the middle of the string? If yes, we need to take care to skip them correctly.

@@ +888,3 @@
+          }
+          years = read;
+          if (years < 0 || years > 584) {       /* see https://bugzilla.gnome.org/show_bug.cgi?id=752336 for rationale */

we need a smarter check than this. We should allow any value for year and detect overflow when computing the duration value.

@@ +899,3 @@
+          }
+          months = read;
+          if (months < 0 || months >= 12) {

the string contains no '-' sign, so months cannot be negative. No need to check for that

@@ +910,3 @@
+          }
+          days = read;
+          if (days < 0 || days >= 31) {

the string contains no '-' sign, so days cannot be negative. No need to check for that

@@ +1058,3 @@
 error:
   xmlFree (prop_string);
+  *property_value = default_value;

don't need this. The gst_mpdparser_parse_duration should not change its value argument if it returns false.
Comment 17 Florin Apostol 2015-10-01 12:29:12 UTC
Review of attachment 312352 [details] [review]:

::: tests/check/elements/dash_mpd.c
@@ +4540,3 @@
+
+  fail_if (gst_mpdparser_parse_duration ("", &v));
+  fail_if (gst_mpdparser_parse_duration (" ", &v));

need to decide of we allow spaces or not. If yes, add some tests with spaces in the middle of the string. For example "P T1s" will fail.

@@ +4579,3 @@
+  fail_if (gst_mpdparser_parse_duration ("PT0", &v));
+  fail_unless (gst_mpdparser_parse_duration ("PT1.1S", &v));
+  fail_if (gst_mpdparser_parse_duration ("PT1.1.1S", &v));

because you use both fail_if and fail_unless, I find it difficult to understand what is the positive and what is the negative testcase.
Something like 
fail_unless (gst_mpdparser_parse_duration (".....", &v) == TRUE);  //for expected success
fail_unless (gst_mpdparser_parse_duration (".....", &v) == FALSE); // for expected failure
might be more clear to the reader.
Comment 18 Vincent Penquerc'h 2015-10-01 14:30:50 UTC
Created attachment 312490 [details] [review]
1/5
Comment 19 Vincent Penquerc'h 2015-10-01 14:31:08 UTC
Created attachment 312491 [details] [review]
2/5
Comment 20 Vincent Penquerc'h 2015-10-01 14:31:27 UTC
Created attachment 312492 [details] [review]
3/5
Comment 21 Vincent Penquerc'h 2015-10-01 14:31:48 UTC
Created attachment 312493 [details] [review]
4/5
Comment 22 Vincent Penquerc'h 2015-10-01 14:32:04 UTC
Created attachment 312494 [details] [review]
5/5
Comment 23 Vincent Penquerc'h 2015-10-01 14:33:35 UTC
Ah, I saw the review after the mail asking for all the patches in upload form. I will make the changes and post them again when done.
Comment 24 Vincent Penquerc'h 2015-10-01 15:12:10 UTC
Created attachment 312497 [details]
4/5 - add checks to duration parsing
Comment 25 Vincent Penquerc'h 2015-10-01 15:12:32 UTC
Created attachment 312498 [details] [review]
5/5 - unit test
Comment 26 Vincent Penquerc'h 2015-10-01 15:13:53 UTC
(In reply to Florin Apostol from comment #16)
> @@ +1058,3 @@
>  error:
>    xmlFree (prop_string);
> +  *property_value = default_value;
> 
> don't need this. The gst_mpdparser_parse_duration should not change its
> value argument if it returns false.

I left this as is, as other parsing functions do this unconditionally at start. Do you think all these functions should be modified to not do it ?
Comment 27 Florin Apostol 2015-10-01 15:22:16 UTC
> I left this as is, as other parsing functions do this unconditionally at
> start. Do you think all these functions should be modified to not do it ?

But this function is also setting *property_value = default_value at start. My comment was that you do not need to do it again in case of errors, because gst_mpdparser_parse_duration should not touch the argument pointer in case of errors.
Comment 28 Vincent Penquerc'h 2015-10-01 15:37:09 UTC
Created attachment 312499 [details] [review]
4/5 - add checks to duration parsing

OK, I added a temporary variable to hold the calculation, and only write it to its destination after all is known good.
Comment 29 Florin Apostol 2015-10-01 16:16:08 UTC
Review of attachment 312499 [details] [review]:

I think it looks great. I don't think we can overflow the duration any more.
Make sure you mark the initial 3 patches as obsolete.

::: ext/dash/gstmpdparser.c
@@ +844,3 @@
+  if (*v > G_MAXUINT64 / mul)
+    return FALSE;
+  *v *= mul;

I suggest you do this in a local variable. A function should not touch output parameters if returning error.
Comment 30 Vincent Penquerc'h 2015-10-01 16:21:51 UTC
Created attachment 312506 [details] [review]
4/5 - add checks to duration parsing
Comment 31 Florin Apostol 2015-10-01 16:25:31 UTC
looks great.
But you made the wrong patches obsolete! You have now 2 patches adding unit tests. You should have kept patches 1-5 and remove the others.
Comment 32 Vincent Penquerc'h 2015-10-01 16:35:00 UTC
Hrm, I thought these three first ones were from another bug, just here so you have the correct base to apply the two new ones. The one from you seems to be from this bug though. I'll update.
Comment 33 Vincent Penquerc'h 2015-10-16 09:51:47 UTC
*** Bug 752337 has been marked as a duplicate of this bug. ***
Comment 34 Vincent Penquerc'h 2015-11-02 11:08:20 UTC
Created attachment 314623 [details] [review]
4/5 - add checks to duration parsing
Comment 35 Vincent Penquerc'h 2015-11-02 11:08:47 UTC
Created attachment 314624 [details] [review]
5/5 - unit test
Comment 36 Vincent Penquerc'h 2015-11-02 11:09:56 UTC
I had to redo the checks patch as it was just conflits.
I also removed the seconds range check, as there is a new use of such in the dash_demux test, and the spec itself shows an example with such as valid.
Comment 37 Vincent Penquerc'h 2015-11-02 11:39:28 UTC
commit 2f8efd1ce3a8ab98606bbda2bd753330474528e3
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Nov 2 10:48:11 2015 +0000

    tests: add a test for MPD file duration parsing
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752336

commit 045a03c14a68f9163a90a9bf448af37ce0ee576f
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Nov 2 10:25:38 2015 +0000

    mpdparser: add some checks to duration parsing
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752336

commit 7dca9fb3f43efe60405c714ecf9dd508f7b61245
Author: Florin Apostol <florin.apostol@oregan.net>
Date:   Tue Sep 29 09:32:02 2015 +0100

    dashdemux: added duration format validation
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752336