Bug 186968 - Fix clang static analyzer warnings: Branch condition evaluates to a garbage value
Summary: Fix clang static analyzer warnings: Branch condition evaluates to a garbage v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-23 06:12 PDT by David Kilzer (:ddkilzer)
Modified: 2018-06-30 14:48 PDT (History)
14 users (show)

See Also:


Attachments
Patch v1 (3.76 KB, patch)
2018-06-23 06:19 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (14.43 KB, patch)
2018-06-24 21:22 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.77 MB, application/zip)
2018-06-24 23:27 PDT, EWS Watchlist
no flags Details
Patch v3 (14.32 KB, patch)
2018-06-25 14:55 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.76 MB, application/zip)
2018-06-26 10:50 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2018-06-23 06:12:18 PDT
Fix clang static analyzer warnings: Branch condition evaluates to a garbage value.
Comment 1 Radar WebKit Bug Importer 2018-06-23 06:12:29 PDT
<rdar://problem/41397137>
Comment 2 David Kilzer (:ddkilzer) 2018-06-23 06:19:35 PDT
Created attachment 343432 [details]
Patch v1
Comment 3 zalan 2018-06-23 06:32:28 PDT
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).
Comment 4 David Kilzer (:ddkilzer) 2018-06-24 21:18:45 PDT
(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.
Comment 5 David Kilzer (:ddkilzer) 2018-06-24 21:22:47 PDT
Created attachment 343483 [details]
Patch v2
Comment 6 zalan 2018-06-24 21:24:09 PDT
(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.
Comment 7 EWS Watchlist 2018-06-24 23:27:37 PDT
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
Comment 8 EWS Watchlist 2018-06-24 23:27:49 PDT
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
Comment 9 David Kilzer (:ddkilzer) 2018-06-25 10:48:26 PDT
(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.
Comment 10 Per Arne Vollan 2018-06-25 11:05:44 PDT
(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.
Comment 11 David Kilzer (:ddkilzer) 2018-06-25 14:55:47 PDT
Created attachment 343545 [details]
Patch v3
Comment 12 David Kilzer (:ddkilzer) 2018-06-25 14:57:15 PDT
(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.
Comment 13 David Kilzer (:ddkilzer) 2018-06-25 14:59:25 PDT
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) {
Comment 14 Said Abou-Hallawa 2018-06-25 15:18:26 PDT
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);
Comment 15 EWS Watchlist 2018-06-26 10:50:25 PDT
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
Comment 16 EWS Watchlist 2018-06-26 10:50:37 PDT
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
Comment 17 zalan 2018-06-26 16:51:40 PDT
(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.
Comment 18 WebKit Commit Bot 2018-06-27 11:07:07 PDT
Comment on attachment 343545 [details]
Patch v3

Clearing flags on attachment: 343545

Committed r233267: <https://trac.webkit.org/changeset/233267>
Comment 19 WebKit Commit Bot 2018-06-27 11:07:09 PDT
All reviewed patches have been landed.  Closing bug.