Bug 206508

Created attachment 388254 [details] Patch v1

Comment on attachment 388254 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=388254&action=review > Source/WTF/ChangeLog:3 > + Enable -Wconditional-uninitialized in WebCore project Hard to tell cost/benefit for this one. Did it spot any problems? > Source/WTF/ChangeLog:13 > + * icu/unicode/utf16.h: > + * icu/unicode/utf_old.h: > + - Initialize `__c2` variable in various macros to workaround > + a potential clang bug in -Wconditional-uninitialized. This > + fixes a warning in WebCore/accessibility/AXObjectCache.cpp, > + but only in external SDK builds. Since these are headers from the ICU project, we would need to upstream the changes to ICU. But perhaps the issue is that we have old versions of these headers? I am not sure it’s OK to change our copies of ICU headers. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:206 > + unsigned vectorLength = 0; How about initializing this to 1 instead of 0. Why is 0 a good value? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:221 > + RELEASE_ASSERT(vectorLength); Not sure adding this is helpful, and if we just start set to 1 that would help. > Source/WebCore/accessibility/AXObjectCache.cpp:2530 > +// FIXME: Remove IGNORE_WARNINGS macros once one of <rdar://problem/58615489&58615391> is fixed. > +IGNORE_WARNINGS_BEGIN("conditional-uninitialized") > U16_NEXT(characterOffset.node->textContent(), offset, characterOffset.node->textContent().length(), ch); > +IGNORE_WARNINGS_END Does this work? If it works then why are we also patching the ICU headers? A patch with both changes doesn’t make logical sense to me. Maybe I am missing something? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1317 > + case CryptoAlgorithmIdentifier::None: > + RELEASE_ASSERT_NOT_REACHED(); > + break; Unneeded, lets not add it. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2352 > + CryptoAlgorithmIdentifier hash = CryptoAlgorithmIdentifier::None; Please just select some actual algorithm identifier to initialize to. No need to add a "none" case. > Source/WebCore/crypto/CryptoAlgorithmIdentifier.h:33 > + None = 0, Please don’t add this. > Source/WebCore/dom/Document.cpp:3618 > - double delay; > + double delay = 0; > String urlString; > if (frame && parseMetaHTTPEquivRefresh(content, delay, urlString)) { OK to make this change. Best fix here is probably to change parseMetaHTTPEquivRefresh to return Optional<X> where X is a struct holding both the delay and the URL string, rather than using out arguments. > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:199 > - GradientData* gradientData; > + GradientData* gradientData = nullptr; > if (m_savedContext && (gradientData = m_gradientMap.get(&renderer))) { The fact that it warns on this seems like a compiler bug.

Comment on attachment 388254 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=388254&action=review >> Source/WTF/ChangeLog:3 >> + Enable -Wconditional-uninitialized in WebCore project > > Hard to tell cost/benefit for this one. Did it spot any problems? Not sure if you're asking about the changes to the WTF project or the WebCore. I will assume WebCore. Yes, it found real issues in WebCore: - In Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp, `vectorLength` could be left unset, leading to an OOB (or unexpected) write when indexing into a data structure. This could happen even if `vectorLength` was set to zero on the stack (due to luck), hence the addition of the RELEASE_ASSERT(). - In Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp, the switch statement had no `default` case for testing a single character value, which would leave `mode` in an undefined state. - In Source/WebCore/css/parser/CSSSupportsParser.cpp, `result` could start in an unknown state (true or false), which could result in an unexpected state returned from the method. >> Source/WTF/ChangeLog:13 >> + but only in external SDK builds. > > Since these are headers from the ICU project, we would need to upstream the changes to ICU. But perhaps the issue is that we have old versions of these headers? I am not sure it’s OK to change our copies of ICU headers. I filed <rdar://problem/58615489> to fix a potential clang bug (false positive) and <rdar://problem/58615391> to fix this in ICU. I will back out this change as the change in Source/WebCore/accessibility/AXObjectCache.cpp below works around this false positive. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:206 >> + unsigned vectorLength = 0; > > How about initializing this to 1 instead of 0. Why is 0 a good value? The code below assumes `vectorLength` is always set to a non-zero value by the for loop. If we set it to 1, we can't detect that scenario. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:221 >> + RELEASE_ASSERT(vectorLength); > > Not sure adding this is helpful, and if we just start set to 1 that would help. As noted above, we can't detect if the for loop above failed to set a value if we set it to 1. There is no guarantee in the loop that `vectorLength` will be set. And since `vectorLength - 1` is used as an array index below, it can never be zero. Additionally, the ASSERT() macros in the loop aren't compiled into Release builds, so if this code is parsing untrusted data, vectorLength could be set to a large 32-bit value if the ASCII character at `innerType.name()[innerType.name().length() - 1]` is less than the '0' character. Those ASSERT() macros in the for loop should probably be extracted into an isValidInnerTypeName() inline function like this: else if (isValidInnerTypeName(...)) { vectorLength = innerType.name()[innerType.name().length() - 1] - '0'; } At minimum, these ASSERT() macros should be ASSERT_WITH_SECURITY_IMPLICATION(), so I'll do that for now. >> Source/WebCore/accessibility/AXObjectCache.cpp:2530 >> +IGNORE_WARNINGS_END > > Does this work? If it works then why are we also patching the ICU headers? A patch with both changes doesn’t make logical sense to me. Maybe I am missing something? This works. I'm patching the ICU headers for time in the future when the ICU headers in the internal SDK are fixed and someone removes these warnings, then forgets that the open source build is not using the internal headers. >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1317 >> + break; > > Unneeded, lets not add it. Will remove. >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2352 >> + CryptoAlgorithmIdentifier hash = CryptoAlgorithmIdentifier::None; > > Please just select some actual algorithm identifier to initialize to. No need to add a "none" case. Will do. >> Source/WebCore/crypto/CryptoAlgorithmIdentifier.h:33 >> + None = 0, > > Please don’t add this. Okay. >> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:199 >> if (m_savedContext && (gradientData = m_gradientMap.get(&renderer))) { > > The fact that it warns on this seems like a compiler bug. Yes, I added this test case to <rdar://problem/58615489> as it appears to be the same issue.

Comment on attachment 388254 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=388254&action=review >>> Source/WTF/ChangeLog:3 >>> + Enable -Wconditional-uninitialized in WebCore project >> >> Hard to tell cost/benefit for this one. Did it spot any problems? > > Not sure if you're asking about the changes to the WTF project or the WebCore. I will assume WebCore. > > Yes, it found real issues in WebCore: > - In Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp, `vectorLength` could be left unset, leading to an OOB (or unexpected) write when indexing into a data structure. This could happen even if `vectorLength` was set to zero on the stack (due to luck), hence the addition of the RELEASE_ASSERT(). > - In Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp, the switch statement had no `default` case for testing a single character value, which would leave `mode` in an undefined state. > - In Source/WebCore/css/parser/CSSSupportsParser.cpp, `result` could start in an unknown state (true or false), which could result in an unexpected state returned from the method. The issue flagged in Source/WebCore/bindings/js/SerializedScriptValue.cpp was a real issue as well since `hash` could be passed uninitialized into CryptoKeyRSA::create().

Comment on attachment 388254 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=388254&action=review > Source/WebCore/css/parser/CSSSupportsParser.cpp:59 > + bool result = false; Below result is only ever set on lines 69 through 74. Is this actually correct? Before &= and |= could have initialized with uninitialized junk, but it's not clear to me that starting with false is correct either.

Created attachment 388373 [details] Patch for landing

Comment on attachment 388254 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=388254&action=review >> Source/WebCore/css/parser/CSSSupportsParser.cpp:59 >> + bool result = false; > > Below result is only ever set on lines 69 through 74. Is this actually correct? Before &= and |= could have initialized with uninitialized junk, but it's not clear to me that starting with false is correct either. Actually, it doesn't matter what the starting value of `result` is because `clauseType` is set to `Unresolved` before the loop starts and thus the `if (clauseType == Unresolved)` condition will evaluate to true the first time through the loop. I just assume that clang isn't able to reason about "co-dependent" variables like this so I picked the zero-value for a `bool` type. Should clang be able to reason about this?

Comment on attachment 388373 [details] Patch for landing Waiting to set cq+ flag until EWS completes.

Created attachment 388377 [details] Patch for landing v2

(In reply to David Kilzer (:ddkilzer) from comment #9) > Created attachment 388377 [details] > Patch for landing v2 ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:213: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:214: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:222: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 3 in 13 files Heh, forgot I added that.

The commit-queue encountered the following flaky tests while processing attachment 388377 [details]: imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 388377 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 388377 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 388377 [details] Patch for landing v2 Clearing flags on attachment: 388377 Committed r255036: <https://trac.webkit.org/changeset/255036>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58846881>