Bug 192805

Summary: [Win] Fix the wrong return value checking of ScriptItemize in UniscribeController::itemizeShapeAndPlace
Product: WebKit Reporter: Fujii Hironori <fujii>
Component: PlatformAssignee: Fujii Hironori <fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, don.olmstead, koivisto, mitz, pvollan, ross.kirsling
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=137526
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Fujii Hironori
Reported 2018-12-17 22:59:38 PST
[Win] Fix the wrong return value checking of ScriptItemize in UniscribeController::itemizeShapeAndPlace UniscribeController::itemizeShapeAndPlace has the following code. > while (rc = ::ScriptItemize(cp, length, m_items.size() - 1, &m_control, &m_state, m_items.data(), &numItems) == E_OUTOFMEMORY) { > m_items.resize(m_items.size() * 2); > resetControlAndState(); > } > if (FAILED(rc)) { > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > return; > } > rc is aligned 0 or 1, and checked by FAILED(rc). '==' has higher operator precedence than '='. It is interpreted as: > rc = (::ScriptItemize(...) == E_OUTOFMEMORY) https://en.cppreference.com/w/cpp/language/operator_precedence This code has been introduced in Bug 137526.
Attachments
Patch (2.37 KB, patch)
2018-12-17 23:24 PST, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (2.85 KB, patch)
2018-12-27 00:22 PST, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (2.86 KB, patch)
2018-12-27 00:24 PST, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Fujii Hironori
Comment 1 2018-12-17 23:09:17 PST
FAILED macro | Microsoft Docs https://docs.microsoft.com/en-us/windows/desktop/api/winerror/nf-winerror-failed > #define FAILED(hr) (((HRESULT)(hr)) < 0) FAILED(rc) is always false. ScriptItemize function | Microsoft Docs https://docs.microsoft.com/en-us/windows/desktop/api/usp10/nf-usp10-scriptitemize > The function returns E_INVALIDARG if pwcInChars is set to NULL, cInChars is 0, pItems is set to NULL, or cMaxItems < 2. If I understand the code correctly, ScriptItemize never returns E_INVALIDARG in above code. There is no way to test.
Fujii Hironori
Comment 2 2018-12-17 23:24:43 PST
Created attachment 357542 [details] Patch
Alex Christensen
Comment 3 2018-12-18 09:19:02 PST
Comment on attachment 357542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357542&action=review The change seems ok. The log seems problematic. > Source/WebCore/ChangeLog:22 > + Fortunately, as far as I understand the code, there is not an > + actual problem, and no way to test the coding error. If this is true we should just remove all this code. > Source/WebCore/ChangeLog:24 > + No new tests, no behavior changes. There is a behavior change.
Fujii Hironori
Comment 4 2018-12-18 22:57:39 PST
Do you mean the ScriptItemize can return E_INVALIDARG in the code? The code has been introduced to resolve static analyzer warnings in Bug 137526. I guess the resolve static analyzer was not smart enough to know ScriptItemize never return E_INVALIDARG in the condition. I guess the resolve static analyzer simply reported E_INVALIDARG is not handled.
Fujii Hironori
Comment 5 2018-12-18 22:59:02 PST
s/the resolve static analyzer/the static analyzer/
Fujii Hironori
Comment 6 2018-12-18 23:13:49 PST
I think the following condition never be true because ScriptItemize never return E_INVALIDARG in the code. > if (FAILED(rc)) { > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > return; > } But, if I remove the above code, the varialbe 'rc' become unsued. Then, I need to remove all 'rc'. the static analyzer ends up reporting the warning again.
Fujii Hironori
Comment 7 2018-12-26 23:41:59 PST
(In reply to Fujii Hironori from comment #6) > I think the following condition never be true because ScriptItemize never > return E_INVALIDARG in the code. > > > if (FAILED(rc)) { > > WTFLogAlways("UniscribeController::itemizeShapeAndPlace: ScriptItemize failed, rc=%lx", rc); > > return; > > } > > But, if I remove the above code, the varialbe 'rc' become unsued. Then, I > need to remove all 'rc'. > the static analyzer ends up reporting the warning again. I'm able to remove the condition by using ASSERT_UNUSED macro. > ASSERT_UNUSED(rc, SUCCEEDED(rc)); // ScriptItemize never returns E_INVALIDARG. The benefit of this approach is there would be no behavior change even if ScriptItemize returns E_INVALIDARG in Release build.
Fujii Hironori
Comment 8 2018-12-27 00:22:01 PST
Created attachment 358103 [details] Patch
Fujii Hironori
Comment 9 2018-12-27 00:24:05 PST
Created attachment 358104 [details] Patch
Fujii Hironori
Comment 10 2019-01-06 19:40:12 PST
Review, please.
Brent Fulgham
Comment 11 2019-01-07 08:51:53 PST
Comment on attachment 358104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358104&action=review > Source/WebCore/platform/graphics/win/UniscribeController.cpp:203 > + ASSERT_UNUSED(rc, SUCCEEDED(rc)); // ScriptItemize must not return E_INVALIDARG in this code. I'm afraid I don't understand the benefit of this change. I know that you found MSDN documentation that claims that ScriptItemize returns E_INVALIDARG in some cases, and is silent about other error conditions. But I don't think that's sufficient evidence to assume that ScriptItemize will never return some other error value (or might be modified in the future to do so). Why not leave this error check in place?
Fujii Hironori
Comment 12 2019-01-07 18:39:11 PST
Thank you for the feedback. Do you prefer the first patch? I have no preference to both approaches. I removed the error checking based on Alex's feedback (Comment 3).
Fujii Hironori
Comment 13 2020-01-13 22:49:11 PST
I removed the code in r254323. Closed.
Note You need to log in before you can comment on or make changes to this bug.