Bug 188556

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>

<rdar://problem/43291961>

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>