Fix Profiler2 gcc7 compiler warning about out of bounds access without breaking the debug build
ClosedPublic
Actions

Authored by elexis on Aug 7 2019, 2:10 PM.

Details

Reviewers: None
Commits: rP22625: Add Profiler2 static const member definition, since the declaration itself is…

Summary

gcc8 has extended array out-of-bounds access warnings, and the memcpy call in rP18423 triggers this:

In file included from /usr/include/string.h:635,
                 from /usr/include/c++/8/cstring:42,
                 from /usr/include/boost/filesystem/path.hpp:36,
                 from /usr/include/boost/filesystem.hpp:16,
                 from ../../../source/lib/pch/pch_boost.h:57,
                 from ../../../source/lib/precompiled.h:76,
                 from ../../../source/pch/engine/precompiled.h:18:
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘void rewriteBuffer(u8*, u32&)’ at ../../../source/ps/Profiler2.cpp:564:10:
/usr/include/x86_64-linux-gnu/bits/string3.h:53:33: warning: ‘void* __builtin___memcpy_chk(void*, const void*, long unsigned int, long unsigned int)’ forming offset [257, 4294967295] is out of the bounds [0, 256] of object ‘message’ with type ‘char [256]’ [-Warray-bounds]
   return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../source/ps/Profiler2.cpp: In function ‘void rewriteBuffer(u8*, u32&)’:
../../../source/ps/Profiler2.cpp:563:9: note: ‘message’ declared here
    char message[CProfiler2::MAX_ATTRIBUTE_LENGTH] = {0};
         ^~~~~~~

The warning should vanish one way or the other.

The warning was fixed in rP22624 but that code does not build in debug mode for a reason unknown to me.

Test Plan

Make sure this doesn't break the gcc debug build (make config=debug). Verify the behavior is the same as before and intended.

That the correct string is processed can be tested by adding a Engine.ProfileAttribute("hello"); to a system simulation component init, running a non-visual replay, and searching in the profile2.jsonp for hello.

One can add a string > the max length in JS and see that it will be truncated with three dots rather than trigger the ENSURE.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 8773
Build 14385: Vulcan Build	Jenkins
Build 14384: arc lint + arc unit

Event Timeline

elexis created this revision.Aug 7 2019, 2:10 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

Harbormaster failed remote builds in B8773: Diff 9260!Aug 7 2019, 2:10 PM

elexis edited the test plan for this revision. (Show Details)Aug 7 2019, 2:32 PM

elexis edited the test plan for this revision. (Show Details)Aug 7 2019, 2:40 PM

The reason as to why this was broken is that std::min requires a const reference, but MAX_ATTRIBUTE_LENGTH is only declared in the header, which doesn't count as a definition, thus no reference.

The same issue is described here: https://stackoverflow.com/questions/16957458/static-const-in-c-class-undefined-reference/16957554

So actually this is needed when using std::min with MAX_ATTRIBUTE_LENGTH:

Index: source/ps/Profiler2.cpp
===================================================================
--- source/ps/Profiler2.cpp	(revision 22624)
+++ source/ps/Profiler2.cpp	(working copy)
@@ -36,6 +36,8 @@
 
 CProfiler2 g_Profiler2;
 
+const size_t CProfiler2::MAX_ATTRIBUTE_LENGTH;
+
 // A human-recognisable pattern (for debugging) followed by random bytes (for uniqueness)
 const u8 CProfiler2::RESYNC_MAGIC[8] = {0x11, 0x22, 0x33, 0x44, 0xf4, 0x93, 0xbe, 0x15};

(So it's clear why it breaks with debug config, but not why it doesnt break with release config.)

The affected function is actually only performed when CProfile2SpikeRegion is destroyed, so I won't push this further cleanup:

Index: source/ps/Profiler2.cpp
===================================================================
--- source/ps/Profiler2.cpp	(revision 22624)
+++ source/ps/Profiler2.cpp	(working copy)
@@ -540,15 +546,16 @@ void rewriteBuffer(u8* buffer, u32& buff
 		{
 			// skip for now
 			u32 len;
 			memcpy(&len, buffer + readPos, sizeof(len));
 			ENSURE(len <= CProfiler2::MAX_ATTRIBUTE_LENGTH);
+			len = std::min(len, static_cast<u32>(CProfiler2::MAX_ATTRIBUTE_LENGTH));
 			readPos += sizeof(len);
 
-			char message[CProfiler2::MAX_ATTRIBUTE_LENGTH] = {0};
-			memcpy(&message[0], buffer + readPos, std::min(size_t(len), CProfiler2::MAX_ATTRIBUTE_LENGTH));
-			CStr mess = CStr((const char*)message, len);
+			char message[len] = {0};
+			memcpy(message, buffer + readPos, len);
+			CStr mess(static_cast<const char*>(message), len);
 			if (!last_names.empty())
 			{
 				timeByTypeMap::iterator it = timeByType.find(std::string(last_names.back()));
 				if (it == timeByType.end())
 					topLevelArgs.insert(mess);