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