Bug 83422

Summary: WebVTT parser unnecessarily limits the value of a timestamp
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: feature-media-reviews, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch mitz: review+

Eric Carlson
Reported 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.
Attachments
Proposed patch (4.24 KB, patch)
2012-04-07 10:10 PDT, Eric Carlson
mitz: review+
Eric Carlson
Comment 1 2012-04-07 10:10:09 PDT
Created attachment 136133 [details] Proposed patch
Radar WebKit Bug Importer
Comment 2 2012-04-07 12:19:55 PDT
Darin Adler
Comment 3 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;
Eric Carlson
Comment 4 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.
Eric Carlson
Comment 5 2012-04-09 09:14:05 PDT
Note You need to log in before you can comment on or make changes to this bug.