83422 2012-04-07 08:33:33 -0700 WebVTT parser unnecessarily limits the value of a timestamp 2012-04-09 09:14:05 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 eric.carlson eric.carlson feature-media-reviews webkit-bug-importer webkit.review.bot oldest_to_newest 597972 0 eric.carlson 2012-04-07 08:33:33 -0700 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. 597982 1 136133 eric.carlson 2012-04-07 10:10:09 -0700 Created attachment 136133 Proposed patch 597995 2 webkit-bug-importer 2012-04-07 12:19:55 -0700 <rdar://problem/11206564> 598004 3 136133 darin 2012-04-07 13:01:06 -0700 Comment on attachment 136133 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; 598315 4 eric.carlson 2012-04-09 09:13:45 -0700 (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. 598317 5 eric.carlson 2012-04-09 09:14:05 -0700 http://trac.webkit.org/changeset/113583 136133 2012-04-07 10:10:09 -0700 2012-04-07 13:01:06 -0700 Proposed patch hours_truncation_patch_1.txt text/plain 4341 eric.carlson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMzU0NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA0LTA3ICBFcmljIENh cmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgorCisgICAgICAgIFdlYlZUVCBwYXJzZXIg dW5uZWNlc3NhcmlseSBsaW1pdHMgdGhlIHZhbHVlIG9mIGEgdGltZXN0YW1wCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04MzQyMgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IG1lZGlhL3RyYWNrL3Ry YWNrLWxhcmdlLXRpbWVzdGFtcC5odG1sCisKKyAgICAgICAgKiBodG1sL3RyYWNrL1dlYlZUVFBh cnNlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXZWJWVFRQYXJzZXI6OmNvbGxlY3RUaW1lU3Rh bXApOiBDYXN0IGhvdXJzIGNvbXBvbmVudCB0byBkb3VibGUgYmVmb3JlIGNhbGN1bGF0aW5nCisg ICAgICAgICAgICBudW1iZXIgb2Ygc2Vjb25kcy4KKwogMjAxMi0wNC0wNyAgUm9iIEJ1aXMgIDxy YnVpc0ByaW0uY29tPgogCiAgICAgICAgIFJlbW92ZSBkZWFkIGNvZGUgaW4gU1ZHQ1NTUGFyc2Vy CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1dlYlZUVFBhcnNlci5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9XZWJWVFRQYXJzZXIuY3BwCShyZXZp c2lvbiAxMTM0MDMpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1dlYlZUVFBhcnNlci5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTMzNCw3ICszMzQsNyBAQCBkb3VibGUgV2ViVlRUUGFyc2Vy Ojpjb2xsZWN0VGltZVN0YW1wKGNvCiAgICAgICAgIHJldHVybiBtYWxmb3JtZWRUaW1lOwogCiAg ICAgLy8gMjAtMjEgLSBDYWxjdWxhdGUgcmVzdWx0LgotICAgIHJldHVybiAodmFsdWUxICogc2Vj b25kc1BlckhvdXIpICsgKHZhbHVlMiAqIHNlY29uZHNQZXJNaW51dGUpICsgdmFsdWUzICsgKChk b3VibGUpdmFsdWU0IC8gMTAwMCk7CisgICAgcmV0dXJuIChzdGF0aWNfY2FzdDxkb3VibGU+KHZh bHVlMSkgKiBzZWNvbmRzUGVySG91cikgKyAodmFsdWUyICogc2Vjb25kc1Blck1pbnV0ZSkgKyB2 YWx1ZTMgKyAoc3RhdGljX2Nhc3Q8ZG91YmxlPih2YWx1ZTQpIC8gMTAwMCk7CiB9CiAKIHZvaWQg V2ViVlRUUGFyc2VyOjpjb25zdHJ1Y3RUcmVlRnJvbVRva2VuKERvY3VtZW50KiBkb2N1bWVudCkK SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDExMzU0NykKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwkod29y a2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDEyLTA0LTA3ICBFcmljIENhcmxzb24gIDxl cmljLmNhcmxzb25AYXBwbGUuY29tPgorCisgICAgICAgIFdlYlZUVCBwYXJzZXIgdW5uZWNlc3Nh cmlseSBsaW1pdHMgdGhlIHZhbHVlIG9mIGEgdGltZXN0YW1wCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04MzQyMgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogbWVkaWEvdHJhY2svY2FwdGlvbnMtd2VidnR0 L2xhcmdlLXRpbWVzdGFtcC52dHQ6IEFkZGVkLgorICAgICAgICAqIG1lZGlhL3RyYWNrL3RyYWNr LWxhcmdlLXRpbWVzdGFtcC1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIG1lZGlhL3Ry YWNrL3RyYWNrLWxhcmdlLXRpbWVzdGFtcC5odG1sOiBBZGRlZC4KKwogMjAxMi0wNC0wNyAgUm9i IEJ1aXMgIDxyYnVpc0ByaW0uY29tPgogCiAgICAgICAgIFJlbW92ZSBkZWFkIGNvZGUgaW4gU1ZH Q1NTUGFyc2VyCkluZGV4OiBMYXlvdXRUZXN0cy9tZWRpYS90cmFjay90cmFjay1sYXJnZS10aW1l c3RhbXAtZXhwZWN0ZWQudHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL21lZGlhL3RyYWNr L3RyYWNrLWxhcmdlLXRpbWVzdGFtcC1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBMYXlv dXRUZXN0cy9tZWRpYS90cmFjay90cmFjay1sYXJnZS10aW1lc3RhbXAtZXhwZWN0ZWQudHh0CShy ZXZpc2lvbiAwKQpAQCAtMCwwICsxLDkgQEAKK1Rlc3RzIHRoYXQgYSB2ZXJ5IGxhcmdlIHRpbWVz dGFtcCBpcyBwYXJzZWQgY29ycmVjdGx5LgorCitFWFBFQ1RFRCAodHJhY2sudHJhY2suY3Vlcy5s ZW5ndGggPT0gJzEnKSBPSworRVhQRUNURUQgKGN1ZS5pZCA9PSAnMScpIE9LCitFWFBFQ1RFRCAo Y3VlLnN0YXJ0VGltZSAvIDYwIC8gNjAgPT0gJzEyMzQ1NjcnKSBPSworRVhQRUNURUQgKGN1ZS5l bmRUaW1lIC8gNjAgLyA2MCA9PSAnMTIzNDU2Nzg5MCcpIE9LCisKK0VORCBPRiBURVNUCisKSW5k ZXg6IExheW91dFRlc3RzL21lZGlhL3RyYWNrL3RyYWNrLWxhcmdlLXRpbWVzdGFtcC5odG1sCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIExheW91dFRlc3RzL21lZGlhL3RyYWNrL3RyYWNrLWxhcmdlLXRpbWVzdGFt cC5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvbWVkaWEvdHJhY2svdHJhY2stbGFy Z2UtdGltZXN0YW1wLmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMzEgQEAKKzwhRE9DVFlQ RSBodG1sPgorPGh0bWw+CisgICAgPGhlYWQ+CisgICAgICAgIDxtZXRhIGh0dHAtZXF1aXY9IkNv bnRlbnQtVHlwZSIgY29udGVudD0idGV4dC9odG1sOyBjaGFyc2V0PXV0Zi04IiAvPgorCisgICAg ICAgIDxzY3JpcHQgc3JjPS4uL21lZGlhLWZpbGUuanM+PC9zY3JpcHQ+CisgICAgICAgIDxzY3Jp cHQgc3JjPS4uL3ZpZGVvLXRlc3QuanM+PC9zY3JpcHQ+CisgICAgICAgIDxzY3JpcHQ+CisKKyAg ICAgICAgICAgIGZ1bmN0aW9uIGxvYWRlZCgpCisgICAgICAgICAgICB7CisgICAgICAgICAgICAg ICAgdHJhY2sgPSBkb2N1bWVudC5nZXRFbGVtZW50c0J5VGFnTmFtZSgidHJhY2siKVswXTsKKyAg ICAgICAgICAgICAgICB0ZXN0RXhwZWN0ZWQoInRyYWNrLnRyYWNrLmN1ZXMubGVuZ3RoIiwgMSk7 CisgICAgICAgICAgICAgICAgY3VlID0gdGVzdFRyYWNrLnRyYWNrLmN1ZXNbMF07CisgICAgICAg ICAgICAgICAgdGVzdEV4cGVjdGVkKCJjdWUuaWQiLCAxKTsKKyAgICAgICAgICAgICAgICB0ZXN0 RXhwZWN0ZWQoImN1ZS5zdGFydFRpbWUgLyA2MCAvIDYwIiwgMTIzNDU2Nyk7CisgICAgICAgICAg ICAgICAgdGVzdEV4cGVjdGVkKCJjdWUuZW5kVGltZSAvIDYwIC8gNjAiLCAxMjM0NTY3ODkwKTsK KyAgICAgICAgICAgICAgICBjb25zb2xlV3JpdGUoIiIpOworCisgICAgICAgICAgICAgICAgZW5k VGVzdCgpOworICAgICAgICAgICAgfQorCisgICAgICAgIDwvc2NyaXB0PgorICAgIDwvaGVhZD4K KyAgICA8Ym9keT4KKyAgICAgICAgPHZpZGVvIGNvbnRyb2xzPgorICAgICAgICAgICAgPHRyYWNr IGlkPSJ0ZXN0VHJhY2siIHNyYz0iY2FwdGlvbnMtd2VidnR0L2xhcmdlLXRpbWVzdGFtcC52dHQi IG9ubG9hZD0ibG9hZGVkKCkiIGRlZmF1bHQ+CisgICAgICAgIDwvdmlkZW8+CisgICAgICAgIDxw PlRlc3RzIHRoYXQgYSB2ZXJ5IGxhcmdlIHRpbWVzdGFtcCBpcyBwYXJzZWQgY29ycmVjdGx5Ljwv cD4KKyAgICA8L2JvZHk+Cis8L2h0bWw+CkluZGV4OiBMYXlvdXRUZXN0cy9tZWRpYS90cmFjay9j YXB0aW9ucy13ZWJ2dHQvbGFyZ2UtdGltZXN0YW1wLnZ0dAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRU ZXN0cy9tZWRpYS90cmFjay9jYXB0aW9ucy13ZWJ2dHQvbGFyZ2UtdGltZXN0YW1wLnZ0dAkocmV2 aXNpb24gMCkKKysrIExheW91dFRlc3RzL21lZGlhL3RyYWNrL2NhcHRpb25zLXdlYnZ0dC9sYXJn ZS10aW1lc3RhbXAudnR0CShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDUgQEAKK++7v1dFQlZUVAor CisxCisxMjM0NTY3OjAwOjAwLjAwMCAtLT4gMTIzNDU2Nzg5MDowMDowMC4wMDAKK0EgdmVyeSBs b25nIGN1ZS4K