Bug 186968 – Fix clang static analyzer warnings: Branch condition evaluates to a garbage value

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Fix clang static analyzer warnings: Branch condition evaluates to a garbage value.

[Radar WebKit Bug Importer]

<rdar://problem/41397137>

[David Kilzer (:ddkilzer)]

Created attachment 343432 [details]
Patch v1

[alan]

Comment on attachment 343432 [details]
Patch v1

Some of them are clearly meant to be optional<>, like the case of isShorthand, if the property is invalid (the early return case) isShorthand should have no value. Also with changing the method signature you could discover other callers with garbage value (emphasisMarkExistsAndIsAbove -> called from paintMarkedTextForeground() and emphasisMarkAbove is not set).

[David Kilzer (:ddkilzer)]

(In reply to zalan from comment #3)
> Comment on attachment 343432 [details]
> Patch v1
> 
> Some of them are clearly meant to be optional<>, like the case of
> isShorthand, if the property is invalid (the early return case) isShorthand
> should have no value. Also with changing the method signature you could
> discover other callers with garbage value (emphasisMarkExistsAndIsAbove ->
> called from paintMarkedTextForeground() and emphasisMarkAbove is not set).

Changing the method signature to use std::optional<bool> totally caught other cases of uninitialized stack variables.  Thanks!!

The only thing I'm not sure about is which style is preferred:

        if (isShorthand.has_value() && isShorthand.value())

Or:

        if (isShorthand.value_or(false))

The second one is more concise, but I find I have to think about its correctness much more than the longer version.

[David Kilzer (:ddkilzer)]

Created attachment 343483 [details]
Patch v2

[alan]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> (In reply to zalan from comment #3)
> > Comment on attachment 343432 [details]
> > Patch v1
> > 
> > Some of them are clearly meant to be optional<>, like the case of
> > isShorthand, if the property is invalid (the early return case) isShorthand
> > should have no value. Also with changing the method signature you could
> > discover other callers with garbage value (emphasisMarkExistsAndIsAbove ->
> > called from paintMarkedTextForeground() and emphasisMarkAbove is not set).
> 
> Changing the method signature to use std::optional<bool> totally caught
> other cases of uninitialized stack variables.  Thanks!!
> 
> The only thing I'm not sure about is which style is preferred:
> 
>         if (isShorthand.has_value() && isShorthand.value())
> 
> Or:
> 
>         if (isShorthand.value_or(false))
> 
> The second one is more concise, but I find I have to think about its
> correctness much more than the longer version.

I usually do
if (isShorthand && *isShorthand)
but not sure if that's the preferred way of doing it.

[EWS Watchlist]

Comment on attachment 343483 [details]
Patch v2

Attachment 343483 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8324535

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 343487 [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

[David Kilzer (:ddkilzer)]

(In reply to Build Bot from comment #7)
> Comment on attachment 343483 [details]
> Patch v2
> 
> Attachment 343483 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/8324535
> 
> New failing tests:
> http/tests/security/video-poster-cross-origin-crash2.html

These tests apparently aren't crashing on Windows bots (although both Release/Debug Windows builds are currently failing.)

<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fpreload%2Fonload_event.html%2C%20http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2FuserAgentShadowDOM%2Fallow-video.html%2C%20http%2Ftests%2Fsecurity%2Fvideo-poster-cross-origin-crash2.html>

No clue how investigate these failures because there aren't any crash logs in the results archive.

[Per Arne Vollan]

(In reply to David Kilzer (:ddkilzer) from comment #9)
> (In reply to Build Bot from comment #7)
> > Comment on attachment 343483 [details]
> > Patch v2
> > 
> > Attachment 343483 [details] did not pass win-ews (win):
> > Output: https://webkit-queues.webkit.org/results/8324535
> > 
> > New failing tests:
> > http/tests/security/video-poster-cross-origin-crash2.html
> 
> These tests apparently aren't crashing on Windows bots (although both
> Release/Debug Windows builds are currently failing.)
> 

This is probably a dupe of https://bugs.webkit.org/show_bug.cgi?id=179297, and is unrelated to this patch.

> <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#tests=http%2Ftests%2Fpreload%2Fonload_event.
> html%2C%20http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2FuserAgentShadowDO
> M%2Fallow-video.html%2C%20http%2Ftests%2Fsecurity%2Fvideo-poster-cross-
> origin-crash2.html>
> 
> No clue how investigate these failures because there aren't any crash logs
> in the results archive.

[David Kilzer (:ddkilzer)]

Created attachment 343545 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to zalan from comment #6)
> I usually do
> if (isShorthand && *isShorthand)
> but not sure if that's the preferred way of doing it.

After sleeping on it, I do like this much better.  It's very concise, and once I know the variable is a std::optional<>, it's easy to recognize the pattern.

[David Kilzer (:ddkilzer)]

Comment on attachment 343545 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=343545&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:1628
> -            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && !emphasisMarkIsAbove) {
> +            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && emphasisMarkIsAbove && !*emphasisMarkIsAbove) {

Make sure to double-check this one.  I wasn't sure if it should be ("unknown or not above"):

    ... && (!emphasisMarkIsAbove || !*emphasisMarkIsAbove)) {

Versus ("known and not above"):

    ... && emphasisMarkIsAbove && !*emphasisMarkIsAbove) {

[Said Abou-Hallawa]

Comment on attachment 343545 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=343545&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:361
> -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const
> +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const

Here is a simpler approach. I think this function can be split into two functions:

bool emphasisMarkExists(const RenderStyle& style)
bool emphasisMarkIsAbove(const RenderStyle& style)

I checked the code and I found all the callers to this function use the boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do not think there a need to have the two functions combined into one function.

> Source/WebCore/rendering/InlineTextBox.cpp:1008
> +    std::optional<bool> emphasisMarkAbove;
>      bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove);
>      const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom();
>      if (!emphasisMark.isEmpty())
> -        emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
> +        emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);

This can be done using two functions like this:

const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ? lineStyle.textEmphasisMarkString() : nullAtom();
if (!emphasisMark.isEmpty())
    emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);

[EWS Watchlist]

Comment on attachment 343545 [details]
Patch v3

Attachment 343545 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8348383

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 343621 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[alan]

(In reply to Said Abou-Hallawa from comment #14)
> Comment on attachment 343545 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343545&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:361
> > -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const
> > +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const
> 
> Here is a simpler approach. I think this function can be split into two
> functions:
> 
> bool emphasisMarkExists(const RenderStyle& style)
> bool emphasisMarkIsAbove(const RenderStyle& style)
> 
> I checked the code and I found all the callers to this function use the
> boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do
> not think there a need to have the two functions combined into one function.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:1008
> > +    std::optional<bool> emphasisMarkAbove;
> >      bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove);
> >      const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom();
> >      if (!emphasisMark.isEmpty())
> > -        emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
> > +        emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
> 
> This can be done using two functions like this:
> 
> const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ?
> lineStyle.textEmphasisMarkString() : nullAtom();
> if (!emphasisMark.isEmpty())
>     emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ?
> -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) :
> font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
I think this should be addressed in a separate patch.

[WebKit Commit Bot]

Comment on attachment 343545 [details]
Patch v3

Clearing flags on attachment: 343545

Committed r233267: <https://trac.webkit.org/changeset/233267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.