Summary: | [JSC] Should not rotate constant with 64 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, don.olmstead, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 188736 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2018-08-14 09:11:08 PDT
Created attachment 347080 [details]
Patch
Comment on attachment 347080 [details]
Patch
Informal review is r+. This issue was found within a run of ARES6 using UBSan. This will prevent the undefined behavior.
Comment on attachment 347080 [details]
Patch
r=me
(In reply to Don Olmstead from comment #2) > Comment on attachment 347080 [details] > Patch > > Informal review is r+. This issue was found within a run of ARES6 using > UBSan. This will prevent the undefined behavior. Yeah, this is nice. (In reply to Mark Lam from comment #3) > Comment on attachment 347080 [details] > Patch > > r=me Thanks :) Committed r234852: <https://trac.webkit.org/changeset/234852> Re-opened since this is blocked by bug 188736 Comment on attachment 347080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347080&action=review > Source/JavaScriptCore/assembler/MacroAssembler.h:1298 > + // Generate the seed in [0, widthInBits). We should not generate widthInBits > + // since it leads to `<< widthInBits`, which is an undefined behavior. > + return random() % (widthInBits - 1); I don’t understand the original bug, nor do I understand how this change will fix it. The comment doesn’t agree with the code. Let’s consider the case where widthInBits is 64. This code then does (u % 63), where u is a 32-bit unsigned integer. That operation should result in an integer in the range [0, 63). The existing code that does (u % 64) already should yield a number in the range [0, 64). The theory of the bug doesn’t make sense. The result of (u % 64) should not ever be 64. Is there a bug here in the original code? How did we come to suspect there was? (In reply to Don Olmstead from comment #2) > This issue was found within a run of ARES6 using > UBSan. This will prevent the undefined behavior. I’d like to know more about exactly what UBSan reported. Comment on attachment 347080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347080&action=review >> Source/JavaScriptCore/assembler/MacroAssembler.h:1298 >> + return random() % (widthInBits - 1); > > I don’t understand the original bug, nor do I understand how this change will fix it. > > The comment doesn’t agree with the code. > > Let’s consider the case where widthInBits is 64. This code then does (u % 63), where u is a 32-bit unsigned integer. That operation should result in an integer in the range [0, 63). The existing code that does (u % 64) already should yield a number in the range [0, 64). The theory of the bug doesn’t make sense. The result of (u % 64) should not ever be 64. > > Is there a bug here in the original code? How did we come to suspect there was? Isn't the bug here just that a shift by a number >= 64 UB? Comment on attachment 347080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347080&action=review >>> Source/JavaScriptCore/assembler/MacroAssembler.h:1298 >>> + return random() % (widthInBits - 1); >> >> I don’t understand the original bug, nor do I understand how this change will fix it. >> >> The comment doesn’t agree with the code. >> >> Let’s consider the case where widthInBits is 64. This code then does (u % 63), where u is a 32-bit unsigned integer. That operation should result in an integer in the range [0, 63). The existing code that does (u % 64) already should yield a number in the range [0, 64). The theory of the bug doesn’t make sense. The result of (u % 64) should not ever be 64. >> >> Is there a bug here in the original code? How did we come to suspect there was? > > Isn't the bug here just that a shift by a number >= 64 UB? I just read the original code. I also don't see how there is a bug. Since we already were doing % 64, we'd end up with a number in the range of [0, 63]. (In reply to Saam Barati from comment #11) > I just read the original code. I also don't see how there is a bug. Since we > already were doing % 64, we'd end up with a number in the range of [0, 63]. Yes, that matches my understanding. Since UBSan complained, I would like to know what it complained about. If it complained about a shift of 64, then I would like to know the details because the results of a % 64 should never be 64. Comment on attachment 347080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347080&action=review >>>> Source/JavaScriptCore/assembler/MacroAssembler.h:1298 >>>> + return random() % (widthInBits - 1); >>> >>> I don’t understand the original bug, nor do I understand how this change will fix it. >>> >>> The comment doesn’t agree with the code. >>> >>> Let’s consider the case where widthInBits is 64. This code then does (u % 63), where u is a 32-bit unsigned integer. That operation should result in an integer in the range [0, 63). The existing code that does (u % 64) already should yield a number in the range [0, 64). The theory of the bug doesn’t make sense. The result of (u % 64) should not ever be 64. >>> >>> Is there a bug here in the original code? How did we come to suspect there was? >> >> Isn't the bug here just that a shift by a number >= 64 UB? > > I just read the original code. I also don't see how there is a bug. Since we already were doing % 64, we'd end up with a number in the range of [0, 63]. Yeah, the problem is, >> 64 AND << 64 are both undefined behavior. https://wiki.sei.cmu.edu/confluence/display/c/INT34-C.+Do+not+shift+an+expression+by+a+negative+number+of+bits+or+by+greater+than+or+equal+to+the+number+of+bits+that+exist+in+the+operand We should generate a seed [1, 63] to make `value = (value << rotation) | (value >> (sizeof(void*) * 8 - rotation));` safe. Comment on attachment 347080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347080&action=review >>>>> Source/JavaScriptCore/assembler/MacroAssembler.h:1298 >>>>> + return random() % (widthInBits - 1); >>>> >>>> I don’t understand the original bug, nor do I understand how this change will fix it. >>>> >>>> The comment doesn’t agree with the code. >>>> >>>> Let’s consider the case where widthInBits is 64. This code then does (u % 63), where u is a 32-bit unsigned integer. That operation should result in an integer in the range [0, 63). The existing code that does (u % 64) already should yield a number in the range [0, 64). The theory of the bug doesn’t make sense. The result of (u % 64) should not ever be 64. >>>> >>>> Is there a bug here in the original code? How did we come to suspect there was? >>> >>> Isn't the bug here just that a shift by a number >= 64 UB? >> >> I just read the original code. I also don't see how there is a bug. Since we already were doing % 64, we'd end up with a number in the range of [0, 63]. > > Yeah, the problem is, >> 64 AND << 64 are both undefined behavior. > https://wiki.sei.cmu.edu/confluence/display/c/INT34-C.+Do+not+shift+an+expression+by+a+negative+number+of+bits+or+by+greater+than+or+equal+to+the+number+of+bits+that+exist+in+the+operand > We should generate a seed [1, 63] to make `value = (value << rotation) | (value >> (sizeof(void*) * 8 - rotation));` safe. The first fix was wrong, since it does not produce 64 without this patch. I thought the UB is caused by `value << 64`. But in fact, it is caused by `value >> (sizeof(void*) * 8 - rotation)` part if `rotation` is 0. I'll upload a patch, which generates a seed within [1, 63]. Created attachment 347474 [details]
Patch
Comment on attachment 347474 [details] Patch Attachment 347474 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8913070 New failing tests: http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html Created attachment 347478 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Committed r235160: <https://trac.webkit.org/changeset/235160> |