[Bug 752336] dashdemux: duration field could overflow

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=752336

--- Comment #16 from Florin Apostol <florin.apostol at oregan.net> ---
Review of attachment 312351:
 --> (https://bugzilla.gnome.org/review?bug=752336&attachment=312351)

::: 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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.