Bug 192805 - [Win] Fix the wrong return value checking of ScriptItemize in UniscribeController::itemizeShapeAndPlace
Summary: [Win] Fix the wrong return value checking of ScriptItemize in UniscribeContro...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-17 22:59 PST by Fujii Hironori
Modified: 2019-01-07 18:39 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.37 KB, patch)
2018-12-17 23:24 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.85 KB, patch)
2018-12-27 00:22 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.86 KB, patch)
2018-12-27 00:24 PST, Fujii Hironori
Hironori.Fujii: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 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.
Comment 1 Fujii Hironori 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.
Comment 2 Fujii Hironori 2018-12-17 23:24:43 PST
Created attachment 357542 [details]
Patch
Comment 3 Alex Christensen 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.
Comment 4 Fujii Hironori 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.
Comment 5 Fujii Hironori 2018-12-18 22:59:02 PST
s/the resolve static analyzer/the static analyzer/
Comment 6 Fujii Hironori 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.
Comment 7 Fujii Hironori 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.
Comment 8 Fujii Hironori 2018-12-27 00:22:01 PST
Created attachment 358103 [details]
Patch
Comment 9 Fujii Hironori 2018-12-27 00:24:05 PST
Created attachment 358104 [details]
Patch
Comment 10 Fujii Hironori 2019-01-06 19:40:12 PST
Review, please.
Comment 11 Brent Fulgham 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?
Comment 12 Fujii Hironori 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).