Bug 192805 – [Win] Fix the wrong return value checking of ScriptItemize in UniscribeController::itemizeShapeAndPlace

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

[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.

[Fujii Hironori]

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]

Created attachment 357542 [details]
Patch

[Alex Christensen]

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]

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]

s/the resolve static analyzer/the static analyzer/

[Fujii Hironori]

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]

(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]

Created attachment 358103 [details]
Patch

[Fujii Hironori]

Created attachment 358104 [details]
Patch

[Fujii Hironori]

Review, please.

[Brent Fulgham]

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]

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]

I removed the code in r254323. Closed.