GNOME Bugzilla – Bug 752336
dashdemux: duration field could overflow
Last modified: 2015-11-02 11:40:16 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.
Created attachment 311556 [details] [review] validation on duration specification
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
(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.
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.
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.
Or 1H1Y since the types are postfix.
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.
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.
(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!
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.
Created attachment 312351 [details] [review] validation on duration specification To be applied on top of your patch.
Created attachment 312352 [details] [review] unit test for duration parsing
(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.
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.
(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.
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.
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.
Created attachment 312490 [details] [review] 1/5
Created attachment 312491 [details] [review] 2/5
Created attachment 312492 [details] [review] 3/5
Created attachment 312493 [details] [review] 4/5
Created attachment 312494 [details] [review] 5/5
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.
Created attachment 312497 [details] 4/5 - add checks to duration parsing
Created attachment 312498 [details] [review] 5/5 - unit test
(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 ?
> 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.
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.
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.
Created attachment 312506 [details] [review] 4/5 - add checks to duration parsing
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.
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.
*** Bug 752337 has been marked as a duplicate of this bug. ***
Created attachment 314623 [details] [review] 4/5 - add checks to duration parsing
Created attachment 314624 [details] [review] 5/5 - unit test
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.
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