[Webkit-unassigned] [Bug 196855] Fix Covscan uninitialized after ctor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=196855

--- Comment #13 from Eike Rathke <erack at redhat.com> ---
(In reply to Michael Catanzaro from comment #7)
> We have several existing Coverity bugs: bug #104114, bug #186798, bug
> #190468. We should be aware of those as well, especially the metabug
> #104114.
I can only see bug #186798 and get "You are not authorized to access" for the others. However, that one bug and patch was submitted by Tomas (I wasn't aware that old bug still existed) and my patch contains the same old fixes if they weren't applied, as I tried to apply everything we have to current master, plus a set of new ones resulting from differences between 2.20.x and 2.22.7

> I would remove the HTMLMediaElement.cpp change from this patch,
> since it's clearly not a UNINIT_CTOR issue, and fix that in a new bug marked
> against bug #104114 instead.
Do you want me to split the patch?

> Also, since there are a relatively large number
> of whitespace fixes in AccessibilityTableColumn.h, those should be landed
> separately as well; if it was a smaller amount, I wouldn't consider it worth
> a separate patch, but that's enough to obscure the fix you've made there.
That's probably due to an editor not saving trailing spaces (which is a good thing, IMHO). And also already in the old patch. But well, yes, that's easily squeezable to the one changed line.

> BTW, welcome Eike. :) Darin, Eike is the new maintainer of Red Hat's WebKit
> and Chromium packages.
Thanks :-)


> > Source/JavaScriptCore/dfg/DFGOSRExit.h:135
> > -    ExtraInitializationLevel extraInitializationLevel;
> > +    ExtraInitializationLevel extraInitializationLevel { };
> 
> You could choose to be explicit here: extraInitializationLevel {
> ExtraInitializationLevel::None };
> 
> But I think this is fine, too.
> 
> > Source/JavaScriptCore/runtime/CodeCache.cpp:141
> > -        JSToken token;
> > +        JSToken token = { };
> 
> I think you should fix this one in ParserTokens.h:
> 
> struct JSToken {
>     JSToken() = default;
>     // ...
> };
> 
> to match the other tokens defined in that file. That would be much more
> robust.
Agreed. That was also discussed in bug #186798. So, what I could do to prevent duplicated discussion and work is to split off all the old changes and submit a new patch that contains only newer fixes. Does that make sense?


> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
> > +        ResultList result { };
> 
> But ResultList is:
> 
> Vector<Value*, 1>
> 
> so this one shouldn't need the brace initialization... correct?
Looks like. That one is also carried over from bug #186798, maybe it was simply "one too much" (three had to be initialized, one not).

> > Source/WebCore/Modules/webaudio/DelayDSPKernel.h:53
> >      int m_writeIndex;
> 
> Both constructors initialize this to 0, so might as well modernize them by
> initializing it here instead of in the constructors' initializer lists.
> 
> > Source/WebCore/Modules/webaudio/DelayDSPKernel.h:56
> >      bool m_firstTime;
> 
> Ditto.
Ditto, both from the old patch.

> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:59
> > +    : m_hasForwardButtonStartPart(false)
> > +    , m_hasForwardButtonEndPart(false)
> > +    , m_hasBackButtonStartPart(false)
> > +    , m_hasBackButtonEndPart(false)
> 
> Hm, is it possible to initialize these in the class declaration instead? Or
> is that not possible, because these are bitfields? If so, this is fine.
Bitfield initializers are available only since C++20


(In reply to Alexey Proskuryakov from comment #11)
> > Although I agree with Darin, I'm also generally of the opinion that we should change WebKit to please as many static analysis tools as we can. Even when the tool reports false positives, as long as it's not too many, then avoiding those will make it easier to see when there are real problems.
> 
> It may be obvious to everyone looking at this bug, but adding initialization
> where it is not needed regresses performance, almost by definition. Other
> changes needed to please code analysis tools can be introducing other
> problems, so I think that each one needs to be discussed on its merits.
If so, then WebKit should be added to the actual Coverity Scan Analysis platform, where each "error" can be marked as false positive (which is remembered) and other means of telling Coverity already in the source code how to interpret things exist. For example, there are dozens of false positives about unreachable code or uninitialized where, well, an ASSERT_NOTREACHED() or so exists in a switch case. Coverity can be taught about such things.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190412/c3bbe3fa/attachment.html>