Page MenuHomeWildfire Games

Fix uninitialized XmppClient members
ClosedPublic

Authored by elexis on Sep 9 2019, 9:40 PM.

Details

Summary

rP21901 and rP22855 failed to initialize primitive members of the class.

Notice that this can also explain why for some Windows 10 users there are wrong TLS cert errors shown if the handshake failed for non-certificate reasons, #4705.

This might or might not fix the conditional jump reported in P170:

==336190== Conditional jump or move depends on uninitialised value(s)
	==336190==    at 0x511AF5D: GetValueType (TypeInference-inl.h:168)
	==336190==    by 0x511AF5D: js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) (TypeInference.cpp:3251)
	==336190==    by 0x4C9FFB3: Monitor (TypeInference-inl.h:556)
	==336190==    by 0x4C9FFB3: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:6166)
	==336190==    by 0x484F0D9: ???
	==336190==    by 0xE3CD19F: ???
	==336190==    by 0x4847809: ???
	==336190==    by 0x4C73A5A: EnterBaseline(JSContext*, js::jit::EnterJitData&) (BaselineJIT.cpp:145)
	==336190==    by 0x4C76F92: js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) (BaselineJIT.cpp:256)
	==336190==    by 0x509547F: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:1787)
	==336190==    by 0x5095656: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:391)
	==336190==    by 0x509591C: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:462)
	==336190==    by 0x50962FB: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:496)
	==336190==    by 0x4EF44B8: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:2790)
	==336190==  Uninitialised value was created by a heap allocation
	==336190==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
	==336190==    by 0x6FA6FB: IXmppClient::create(ScriptInterface const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, bool) (XmppClient.cpp:66)
	==336190==    by 0x6F66DD: JSI_Lobby::StartXmppClient(ScriptInterface::CxPrivate*, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&, int) (JSInterface_Lobby.cpp:96)
	==336190==    by 0x6F83D3: call<void(ScriptInterface::CxPrivate*, const std::__cxx11::basic_string<wchar_t>&, const std::__cxx11::basic_string<wchar_t>&, const std::__cxx11::basic_string<wchar_t>&, const std::__cxx11::basic_string<wchar_t>&, int), std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, int> (NativeWrapperDefns.h:85)
	==336190==    by 0x6F83D3: bool ScriptInterface::call<void, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, int, &JSI_Lobby::StartXmppClient>(JSContext*, unsigned int, JS::Value*) (NativeWrapperDefns.h:124)
	==336190==    by 0x50959E7: CallJSNative (jscntxtinlines.h:235)
	==336190==    by 0x50959E7: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:444)
	==336190==    by 0x508B4DB: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2766)
	==336190==    by 0x5095656: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:391)
	==336190==    by 0x509591C: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:462)
	==336190==    by 0x50962FB: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:496)
	==336190==    by 0x4EF44B8: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:2790)
	==336190==    by 0x6544DF: IGUIObject::ScriptEvent(CStr8 const&) (IGUIObject.cpp:421)
	==336190==    by 0x652CB7: IGUIObject::SendEvent(EGUIMessageType, CStr8 const&) (IGUIObject.cpp:391)
Test Plan

Join the lobby with valgrind and check for the error if and only if applying the patch. Notice it can make a difference whether you compiled with clang or gcc.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 9 2019, 9:40 PM
Vulcan added a comment.Sep 9 2019, 9:42 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/120/display/redirect

elexis edited the summary of this revision. (Show Details)Sep 9 2019, 9:43 PM
elexis edited the test plan for this revision. (Show Details)
Vulcan added a comment.Sep 9 2019, 9:45 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/629/display/redirect

elexis edited the summary of this revision. (Show Details)Sep 9 2019, 10:38 PM
elexis edited the test plan for this revision. (Show Details)

I've tested more, I surrender.

This takes too long, hours in fact.

Sometimes the warning is there, other times it is not.
Sometimes it seems like it depends on clang, sometimes on gcc.
Sometimes it seems like it depends on the XmppClient ctor, other times it looks like glooxwrapper.

The member init was missing and it can explain the TLS errors, screw the conditional jump.

elexis added inline comments.Sep 9 2019, 10:44 PM
source/lobby/XmppClient.cpp
94 ↗(On Diff #9696)

(and no m_PresenceUpdate does not have inconsistent capitalization, since it is more logically related to the other members that are stl containers, for example m_GuiMessageQueue)

This revision was not accepted when it landed; it landed in state Needs Review.Sep 9 2019, 10:50 PM
This revision was automatically updated to reflect the committed changes.

(Here another example disconnect msg by nani that really looks like the strings are random.)

Stan added a subscriber: Stan.EditedSep 11 2019, 10:07 AM

Add "This may be because of one of the following reasons:" and a few bullet points and it's solved.

elexis added a comment.EditedSep 11 2019, 10:24 AM
In D2278#94794, @Stan wrote:

Add "This may be because of one of the following reasons:" and a few bullet points and it's solved.

The wrong messages are (should be) fixed by this commit.