Bug 83422 - WebVTT parser unnecessarily limits the value of a timestamp
Summary: WebVTT parser unnecessarily limits the value of a timestamp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-07 08:33 PDT by Eric Carlson
Modified: 2012-04-09 09:14 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (4.24 KB, patch)
2012-04-07 10:10 PDT, Eric Carlson
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-04-07 08:33:33 PDT
The WebVTT spec defines the hour portion of a timestamp as: Two or more characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9), representing the hours as a base ten integer.

WebKit's WebVTT parser uses an integer to represent hours and converts to a float by multiplying with another integer, limiting the maximum number of hours to 596,523 ((2**31) - 1) / 3600. Because the spec allows more than 6 digits for an hour, this is a bug.

Thanks to Ralph Giles for reporting this.
Comment 1 Eric Carlson 2012-04-07 10:10:09 PDT
Created attachment 136133 [details]
Proposed patch
Comment 2 Radar WebKit Bug Importer 2012-04-07 12:19:55 PDT
<rdar://problem/11206564>
Comment 3 Darin Adler 2012-04-07 13:01:06 PDT
Comment on attachment 136133 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136133&action=review

> Source/WebCore/html/track/WebVTTParser.cpp:337
> +    return (static_cast<double>(value1) * secondsPerHour) + (value2 * secondsPerMinute) + value3 + (static_cast<double>(value4) / 1000);

Why not cast of value2?

Another way to fix this is to make secondsPerHour and secondsPerMinute both double constants instead of in, and also make a third constant called secondsPerMillisecond (a double with the value 0.001) and write this:

    return value1 * secondsPerHour + value2 * secondsPerMinute + value3 + value4 * secondsPerMillisecond;
Comment 4 Eric Carlson 2012-04-09 09:13:45 PDT
(In reply to comment #3)
> (From update of attachment 136133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136133&action=review
> 
> > Source/WebCore/html/track/WebVTTParser.cpp:337
> > +    return (static_cast<double>(value1) * secondsPerHour) + (value2 * secondsPerMinute) + value3 + (static_cast<double>(value4) / 1000);
> 
> Why not cast of value2?
> 
value2 is always between 0 and 59 so it did not need a cast.

> Another way to fix this is to make secondsPerHour and secondsPerMinute both double constants instead of in, and also make a third constant called secondsPerMillisecond (a double with the value 0.001) and write this:
> 
>     return value1 * secondsPerHour + value2 * secondsPerMinute + value3 + value4 * secondsPerMillisecond;

Thanks for the great suggestion, I made this change.
Comment 5 Eric Carlson 2012-04-09 09:14:05 PDT
http://trac.webkit.org/changeset/113583