Page MenuHomeWildfire Games

Fix undefined behavior in the StunClient
ClosedPublic

Authored by elexis on Nov 25 2017, 1:37 PM.

Details

Summary

As reported leper in rP19703, when compiling with clang we get some warnings. Most importantly the undefined behavior when shifting 8 bits of a byte type.

Test Plan

I quickly tested that STUN connections still succeed. But please make sure that I didn't change the intended behavior of the byte getter.

Event Timeline

elexis created this revision.Nov 25 2017, 1:37 PM
Vulcan added a subscriber: Vulcan.Nov 25 2017, 2:56 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
leper added inline comments.Nov 25 2017, 10:20 PM
source/lobby/glooxwrapper/glooxwrapper.h
641 ↗(On Diff #4364)

This seems strange. I suspect not actually handling this properlymight make make us leak memory (to be more specific m_Wrapped).

source/network/StunClient.cpp
127

I guess one could provide a specialization of the other function so someone calling that with u8 does not run into the same issue again.

elexis added a comment.Dec 2 2017, 2:18 PM

You are right about a specialization (I'm not too familiar with templates yet).

source/lobby/glooxwrapper/glooxwrapper.h
641 ↗(On Diff #4364)

Perhaps nicer in a separate memory leak investigation patch if we can fix the other two things beforehand

source/network/StunClient.cpp
127

Since n is a deduced argument, it could only be

template<u8>
bool GetFromBuffer(const std::vector<u8>& buffer, u32& offset, u8& result)

But then I still have the clang compile warning.

Since this function copies one line, we can just keep that one function and add that n=1 special case to that function if we can't resolve the specialization however.

I believe it might be possible to do something with template constraints http://en.cppreference.com/w/cpp/language/constraints (search for size on that page), but that looks alien and might even end up more complicated and longer.

elexis updated this revision to Diff 4491.Dec 2 2017, 2:29 PM

Minimize code, don't do glooxwrapper in this diff

Vulcan added a comment.Dec 2 2017, 2:35 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 2 2017, 2:36 PM
Executing section Default...
Executing section Source...
Executing section JS...
fcxSanya accepted this revision.Dec 3 2017, 2:57 PM

These 3 methods (AddUInt16, AddUInt32, GetFromBuffer) are copied from STK's BareNetworkString. I didn't copy the entire class since I tried to extract the necessary code only, hoping to replace these general methods with pyrogenesis' own or library counterparts.

I think it would be confusing to call GetFromBuffer with non-empty result param and n different from the result size in any case (f.e. reading one more byte into uint16), so the first shift (when a = n - 1) shouldn't be needed.

This revision is now accepted and ready to land.Dec 3 2017, 2:57 PM
elexis added a comment.Dec 4 2017, 2:13 PM

@fcxSanya you mean replacing if (n > 1) with if (a < n - 1) check so that the very first time entering the loop we skip the bitshift, thus fix the issue for the byte type and also preventing any useless bitshift beyond the type size for any other type?
Sounds preferable to me because we can delete the comment then.

In D1064#43977, @elexis wrote:

you mean replacing if (n > 1) with if (a < n - 1) check

I just said that either your uint8-specific if (n > 1) or a more generic if (a < n - 1) should be fine given the method expected usage.

If you will replace the check, wouldn't you still get the compiler warning? (I'm not sure how intelligent the compiler is in calculating those)

elexis added a comment.Dec 4 2017, 3:16 PM

Oh you are right, the compiler is too stupid. Shame. Thanks for the review fcxSanya!

This revision was automatically updated to reflect the committed changes.