Bug 186798 – [JSC] Coverity scan issues

• Component: JavaScriptCore
• Status: NEW

[Tomas Popela]

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:879: member_not_init_in_gen_ctor: The compiler-generated constructor for this class does not initialize "m_positionInMoveList".
#  877|   
#  878|       private:
#  879|->         unsigned m_positionInMoveList;
#  880|           Vector<unsigned, 0, UnsafeVectorOverflow> m_moveList;
#  881|           Vector<unsigned, 0, UnsafeVectorOverflow> m_lowPriorityMoveList;

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/bytecode/PolymorphicAccess.h:125: member_not_init_in_gen_ctor: The compiler-generated constructor for this class does not initialize "m_kind".
#  123|       
#  124|   private:
#  125|->     Kind m_kind;
#  126|       MacroAssemblerCodePtr m_code;
#  127|       Vector<std::pair<InlineWatchpointSet&, StringFireDetail>> m_watchpointsToFire;

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/dfg/DFGOSRExit.h:134: member_decl: Class member declaration for "extraInitializationLevel".
webkitgtk-2.20.3/Source/JavaScriptCore/dfg/DFGOSRExit.h:120: uninit_member: Non-static class member "extraInitializationLevel" is not initialized in this constructor nor in any functions that it calls.
#  118|           , jumpTarget(jumpTarget)
#  119|           , arrayProfile(arrayProfile)
#  120|->     { }
#  121|   
#  122|       OSRExitBase& exit;

Error: CHECKED_RETURN (CWE-252):
webkitgtk-2.20.3/Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:512: check_return: Calling "getString" without checking return value (as is done elsewhere 64 out of 75 times).
webkitgtk-2.20.3/Source/WebDriver/CommandResult.cpp:60: example_checked: Example 1: "errorObject->getString(WTF::String const("message"), errorMessage)" has its value checked in "errorObject->getString(WTF::String const("message"), errorMessage)".
webkitgtk-2.20.3/Source/WebDriver/Session.cpp:778: example_checked: Example 2: "valueObject->getString(WTF::String(WTF::StringAppend<char const *, WTF::String>("session-node-" + this->id()).operator String()), elementID)" has its value checked in "valueObject->getString(WTF::String(WTF::StringAppend<char const *, WTF::String>("session-node-" + this->id()).operator String()), elementID)".
webkitgtk-2.20.3/Source/WebDriver/Session.cpp:917: example_checked: Example 3: "response.responseObject->getString(WTF::String const(WTF::ASCIILiteral("result")), valueString)" has its value checked in "response.responseObject->getString(WTF::String const(WTF::ASCIILiteral("result")), valueString)".
webkitgtk-2.20.3/Source/WebDriver/Session.cpp:2113: example_checked: Example 4: "response.responseObject->getString(WTF::String const(WTF::ASCIILiteral("message")), valueString)" has its value checked in "response.responseObject->getString(WTF::String const(WTF::ASCIILiteral("message")), valueString)".
webkitgtk-2.20.3/Source/WebDriver/Session.cpp:485: example_checked: Example 5: "browsingContext->getString(WTF::String const(WTF::ASCIILiteral("handle")), handle)" has its value checked in "browsingContext->getString(WTF::String const(WTF::ASCIILiteral("handle")), handle)".
#  510|       RefPtr<JSON::Array> actions;
#  511|       if (options) {
#  512|->         options->getString(ASCIILiteral("condition"), condition);
#  513|           options->getBoolean(ASCIILiteral("autoContinue"), autoContinue);
#  514|           options->getArray(ASCIILiteral("actions"), actions);

Error: UNINIT (CWE-457):
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:132: var_decl: Declaring variable "token".
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:133: uninit_use_in_call: Using uninitialized value "token.m_type" when calling "JSToken".
webkitgtk-2.20.3/Source/JavaScriptCore/parser/ParserTokens.h:252:8: read_parm_fld: Reading a parameter field.
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:133: uninit_use_in_call: Using uninitialized value "token.m_data" when calling "JSToken".
webkitgtk-2.20.3/Source/JavaScriptCore/parser/ParserTokens.h:252:8: read_parm_fld: Reading a parameter field.
#  131|       if (UNLIKELY(!statement)) {
#  132|           JSToken token;
#  133|->         error = ParserError(ParserError::SyntaxError, ParserError::SyntaxErrorIrrecoverable, token, "Parser error", -1);
#  134|           return nullptr;
#  135|       }

Error: UNINIT (CWE-457):
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:140: var_decl: Declaring variable "token".
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:141: uninit_use_in_call: Using uninitialized value "token.m_type" when calling "JSToken".
webkitgtk-2.20.3/Source/JavaScriptCore/parser/ParserTokens.h:252:8: read_parm_fld: Reading a parameter field.
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/CodeCache.cpp:141: uninit_use_in_call: Using uninitialized value "token.m_data" when calling "JSToken".
webkitgtk-2.20.3/Source/JavaScriptCore/parser/ParserTokens.h:252:8: read_parm_fld: Reading a parameter field.
#  139|       if (UNLIKELY(!funcDecl)) {
#  140|           JSToken token;
#  141|->         error = ParserError(ParserError::SyntaxError, ParserError::SyntaxErrorIrrecoverable, token, "Parser error", -1);
#  142|           return nullptr;
#  143|       }

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/ConfigFile.cpp:236: member_decl: Class member declaration for "m_file".
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/ConfigFile.cpp:60: uninit_member: Non-static class member "m_file" is not initialized in this constructor nor in any functions that it calls.
#   58|           m_srcPtr = &m_buffer[0];
#   59|           m_bufferEnd = &m_buffer[0];
#   60|->     }
#   61|   
#   62|       bool start()

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/JSBigInt.h:137: member_decl: Class member declaration for "m_sign".
webkitgtk-2.20.3/Source/JavaScriptCore/runtime/JSBigInt.cpp:75: uninit_member: Non-static class member "m_sign" is not initialized in this constructor nor in any functions that it calls.
#   73|       : Base(vm, structure)
#   74|       , m_length(length)
#   75|-> { }
#   76|   
#   77|   void JSBigInt::initialize(InitializationType initType)

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:150: member_decl: Class member declaration for "blockType".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:96: uninit_member: Non-static class member "blockType" is not initialized in this constructor nor in any functions that it calls.
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:151: member_decl: Class member declaration for "continuation".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:96: uninit_member: Non-static class member "continuation" is not initialized in this constructor nor in any functions that it calls.
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:152: member_decl: Class member declaration for "special".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:96: uninit_member: Non-static class member "special" is not initialized in this constructor nor in any functions that it calls.
#   94|           ControlData()
#   95|           {
#   96|->         }
#   97|   
#   98|           void dump(PrintStream& out) const

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmFormat.h:229: member_decl: Class member declaration for "m_initial".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmFormat.h:212: uninit_member: Non-static class member "m_initial" is not initialized in this constructor nor in any functions that it calls.
#  210|       {
#  211|           ASSERT(!*this);
#  212|->     }
#  213|   
#  214|       TableInformation(uint32_t initial, std::optional<uint32_t> maximum, bool isImport)

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmFunctionParser.h:90: member_decl: Class member declaration for "m_currentOpcode".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmFunctionParser.h:106: uninit_member: Non-static class member "m_currentOpcode" is not initialized in this constructor nor in any functions that it calls.
#  104|           dataLogLn("Parsing function starting at: ", (uintptr_t)functionStart, " of length: ", functionLength);
#  105|       m_context.setParser(this);
#  106|-> }
#  107|   
#  108|   template<typename Context>

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/Atomics.h:159: member_decl: Class member declaration for "value".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmTierUpCount.h:54: uninit_member: Non-static class member field "m_tierUpStarted.value" is not initialized in this constructor nor in any functions that it calls.
#   52|           ASSERT(other.m_count == Options::webAssemblyOMGTierUpCount());
#   53|           m_count = other.m_count;
#   54|->     }
#   55|   
#   56|       static uint32_t loopDecrement() { return Options::webAssemblyLoopDecrement(); }

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmValidate.cpp:75: member_decl: Class member declaration for "m_blockType".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmValidate.cpp:48: uninit_member: Non-static class member "m_blockType" is not initialized in this constructor nor in any functions that it calls.
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmValidate.cpp:76: member_decl: Class member declaration for "m_signature".
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/WasmValidate.cpp:48: uninit_member: Non-static class member "m_signature" is not initialized in this constructor nor in any functions that it calls.
#   46|           ControlData()
#   47|           {
#   48|->         }
#   49|   
#   50|           void dump(PrintStream& out) const

Error: UNINIT (CWE-457):
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/js/WasmToJS.cpp:267: var_decl: Declaring variable "realResult" without initializer.
webkitgtk-2.20.3/Source/JavaScriptCore/wasm/js/WasmToJS.cpp:288: uninit_use: Using uninitialized value "realResult".
#  286|   
#  287|                   RETURN_IF_EXCEPTION(throwScope, 0);
#  288|->                 return realResult;
#  289|               };
#  290|           

Error: UNINIT_CTOR (CWE-456):
webkitgtk-2.20.3/Source/JavaScriptCore/yarr/YarrJIT.h:204: member_not_init_in_gen_ctor: The compiler-generated constructor for this class does not initialize "m_usesPatternContextBuffer".
#  202|       MacroAssemblerCodeRef m_matchOnly16;
#  203|   #if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
#  204|->     bool m_usesPatternContextBuffer;
#  205|   #endif
#  206|       std::optional<JITFailureReason> m_failureReason;

[Tomas Popela]

Created attachment 343029 [details]
Patch

[EWS Watchlist]

Comment on attachment 343029 [details]
Patch

Attachment 343029 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/8244735

New failing tests:
inspector/debugger/setBreakpoint-options-exception.html
inspector/debugger/setBreakpoint-autoContinue.html

[EWS Watchlist]

Created attachment 343036 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 343029 [details]
Patch

Attachment 343029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8244808

New failing tests:
inspector/debugger/setBreakpoint-options-exception.html
inspector/debugger/setBreakpoint-autoContinue.html
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 343040 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 343029 [details]
Patch

Attachment 343029 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8244774

New failing tests:
inspector/debugger/setBreakpoint-options-exception.html
inspector/debugger/setBreakpoint-autoContinue.html

[EWS Watchlist]

Created attachment 343042 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Alexey Proskuryakov]

Comment on attachment 343029 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] Coverity scan issues

Does this fix any bugs?

[Tomas Popela]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 343029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343029&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [JSC] Coverity scan issues
> 
> Does this fix any bugs?

No, nothing I'm aware of. But it would be nice to see these things fixed.

[Joseph Pecoraro]

Comment on attachment 343029 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:514
> -        options->getString(ASCIILiteral("condition"), condition);
> +        if (!options->getString(ASCIILiteral("condition"), condition))
> +            return;
>          options->getBoolean(ASCIILiteral("autoContinue"), autoContinue);
>          options->getArray(ASCIILiteral("actions"), actions);

Why only `getString` and not the `getBoolean` and others?

> Source/JavaScriptCore/runtime/CodeCache.cpp:132
> -        JSToken token;
> +        JSToken token = { };

Does this change something?

[Fujii Hironori]

(In reply to Joseph Pecoraro from comment #10)
> > Source/JavaScriptCore/runtime/CodeCache.cpp:132
> > -        JSToken token;
> > +        JSToken token = { };
> 
> Does this change something?

This code zero-initializes the JSToken struct.

https://en.cppreference.com/w/cpp/language/aggregate_initialization

> If the number of initializer clauses is less than the number of
> members or initializer list is completely empty, the remaining
> members are value-initialized. If a member of a reference type is
> one of these remaining members, the program is ill-formed.

https://en.cppreference.com/w/cpp/language/value_initialization

> 4) otherwise, the object is zero-initialized.

[Mark Lam]

(In reply to Fujii Hironori from comment #11)
> (In reply to Joseph Pecoraro from comment #10)
> > > Source/JavaScriptCore/runtime/CodeCache.cpp:132
> > > -        JSToken token;
> > > +        JSToken token = { };
> > 
> > Does this change something?
> 
> This code zero-initializes the JSToken struct.
> 
> https://en.cppreference.com/w/cpp/language/aggregate_initialization

This is only needed because JSToken doesn't have a default constructor that initializes the fields.  Why not add such a default constructor instead and fix an entire class of bugs (plus not having to use the assignment initializer which looks weird)?

[Joseph Pecoraro]

(In reply to Mark Lam from comment #12)
> (In reply to Fujii Hironori from comment #11)
> > (In reply to Joseph Pecoraro from comment #10)
> > > > Source/JavaScriptCore/runtime/CodeCache.cpp:132
> > > > -        JSToken token;
> > > > +        JSToken token = { };
> > > 
> > > Does this change something?
> > 
> > This code zero-initializes the JSToken struct.
> > 
> > https://en.cppreference.com/w/cpp/language/aggregate_initialization
> 
> This is only needed because JSToken doesn't have a default constructor that
> initializes the fields.  Why not add such a default constructor instead and
> fix an entire class of bugs (plus not having to use the assignment
> initializer which looks weird)?

Yeah, this is exactly what I was expecting.

[Tomas Popela]

(In reply to Joseph Pecoraro from comment #10)
> > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:514
> > -        options->getString(ASCIILiteral("condition"), condition);
> > +        if (!options->getString(ASCIILiteral("condition"), condition))
> > +            return;
> >          options->getBoolean(ASCIILiteral("autoContinue"), autoContinue);
> >          options->getArray(ASCIILiteral("actions"), actions);
> 
> Why only `getString` and not the `getBoolean` and others?

True, let's skip that one for this patch and I will prepare a patch getString(), getBoolean and others.

(In reply to Mark Lam from comment #12)
> Why not add such a default constructor instead and
> fix an entire class of bugs (plus not having to use the assignment
> initializer which looks weird)?

This definitely sound like a better solution. I will update the patch.

[Tomas Popela]

Created attachment 345837 [details]
Patch

In the end I didn't used the default contructor, but the member initializer

[Mark Lam]

Comment on attachment 345837 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:151
> +        BlockType blockType { };

Is this really better?  AFAICT, this only initializes these variables to some random value, not necessarily the correct value for the client use case.  For value types like these that have no meaningful default value, I think it's better to have a constructor that initializes the value and force the client to specify the correct value.  Is that not an option?

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:90
> +    OpType m_currentOpcode { };

Ditto.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:76
> +        BlockType m_blockType { };
> +        Type m_signature { };

Ditto.

[Saam Barati]

Comment on attachment 345837 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.h:-124
> -    

Please revert all the whitspace changes

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:90
>> +    OpType m_currentOpcode { };
> 
> Ditto.

I think this would initialize to the opcode for zero, no?

[Filip Pizlo]

Comment on attachment 345837 [details]
Patch

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

Please revert the whitespace changes.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:151
>> +        BlockType blockType { };
> 
> Is this really better?  AFAICT, this only initializes these variables to some random value, not necessarily the correct value for the client use case.  For value types like these that have no meaningful default value, I think it's better to have a constructor that initializes the value and force the client to specify the correct value.  Is that not an option?

{ } means initialize to the actual T() value, not the undef ("random") value.

For example:

int x = { };

Means:

int x = 0;

>>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:90
>>> +    OpType m_currentOpcode { };
>> 
>> Ditto.
> 
> I think this would initialize to the opcode for zero, no?

Yeah it will.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:76
>> +        Type m_signature { };
> 
> Ditto.

This is right, it initializes to zero.

[Tomas Popela]

Huh, I completely forget about this one.

(In reply to Filip Pizlo from comment #18)
> Please revert the whitespace changes.

OK, I will.

[Tomas Popela]

Created attachment 367411 [details]
Patch