Merge pull request teeworlds#3155 from Robyt3/Clang-Tidy

Add clang-tidy to CI and enable clang-analyzer checks

Author: oy

.clang-tidy

@@ -0,0 +1,46 @@
+# Why we disabled individual checks:
+#
+# clang-analyzer-optin.cplusplus.UninitializedObject
+#   TODO: Occurs commonly in graphics_threaded.h
+# clang-analyzer-optin.cplusplus.VirtualCall
+#   Occurs very commonly all over
+# clang-analyzer-optin.performance.Padding
+#   Too annoying to always align for perfect padding
+# clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,
+#   TODO: Requires C11 to fix
+# misc-unused-parameters
+#   TODO: Many changes
+
+Checks: >
+  -*,
+  bugprone-*,
+  -bugprone-assignment-in-if-condition,
+  -bugprone-branch-clone,
+  -bugprone-easily-swappable-parameters,
+  -bugprone-implicit-widening-of-multiplication-result,
+  -bugprone-incorrect-roundings,
+  -bugprone-integer-division,
+  -bugprone-macro-parentheses,
+  -bugprone-narrowing-conversions,
+  -bugprone-parent-virtual-call,
+  -bugprone-reserved-identifier,
+  -bugprone-suspicious-include,
+  -bugprone-unhandled-self-assignment,
+  clang-analyzer-*,
+  -clang-analyzer-optin.cplusplus.UninitializedObject,
+  -clang-analyzer-optin.cplusplus.VirtualCall,
+  -clang-analyzer-optin.performance.Padding,
+  -clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,
+  cppcoreguidelines-avoid-goto,
+  cppcoreguidelines-interfaces-global-init,
+  cppcoreguidelines-slicing,
+  cppcoreguidelines-virtual-class-destructor,
+  misc-*,
+  -misc-const-correctness,
+  -misc-no-recursion,
+  -misc-non-private-member-variables-in-classes,
+  -misc-static-assert,
+  -misc-unused-parameters,
+  performance-*,
+  -performance-no-int-to-ptr,
+  portability-*,

.github/workflows/clang-tidy.yml

@@ -0,0 +1,29 @@
+name: Check clang-tidy
+
+on:
+  push:
+    branches-ignore:
+      - master
+      - staging.tmp
+      - trying.tmp
+      - staging-squash-merge.tmp
+  pull_request:
+
+jobs:
+  check-clang-tidy:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        submodules: true
+
+    - name: Install clang-tidy
+      run: |
+        sudo apt-get update -y
+        sudo apt-get install pkg-config cmake ninja-build libfreetype6-dev libsdl2-dev clang-tidy -y
+    - name: Build with clang-tidy
+      run: |
+        mkdir clang-tidy
+        cd clang-tidy
+        cmake -G Ninja -DCMAKE_CXX_CLANG_TIDY="clang-tidy;-warnings-as-errors=*" -DCMAKE_C_CLANG_TIDY="clang-tidy;-warnings-as-errors=*" -DCMAKE_BUILD_TYPE=Debug -Werror=dev ..
+        cmake --build . --config Debug --target everything -- -k 0

CMakeLists.txt

@@ -137,9 +137,6 @@ if(NOT MSVC)
   # Protect the stack pointer.
   add_c_compiler_flag_if_supported(OUR_FLAGS -fstack-protector-all)
 
-  # Protect the stack from clashing.
-  add_c_compiler_flag_if_supported(OUR_FLAGS -fstack-clash-protection)
-
   # Control-flow protection. Should protect against ROP.
   add_c_compiler_flag_if_supported(OUR_FLAGS -fcf-protection)
 

src/base/system.c

@@ -115,7 +115,11 @@ void dbg_assert_imp(const char *filename, int line, int test, const char *msg)
 
 void dbg_break()
 {
-	*((volatile unsigned*)0) = 0x0;
+#ifdef __GNUC__
+	__builtin_trap();
+#else
+	abort();
+#endif
 }
 
 void dbg_msg(const char *sys, const char *fmt, ...)
@@ -148,20 +152,20 @@ void dbg_msg(const char *sys, const char *fmt, ...)
 #if defined(CONF_FAMILY_WINDOWS)
 static void logger_win_console(const char *line, void *user)
 {
-	#define _MAX_LENGTH 1024
-	#define _MAX_LENGTH_ERROR (_MAX_LENGTH+32)
+	#define MAX_LENGTH 1024
+	#define MAX_LENGTH_ERROR (MAX_LENGTH+32)
 
 	static const int UNICODE_REPLACEMENT_CHAR = 0xfffd;
 
 	static const char *STR_TOO_LONG = "(str too long)";
 	static const char *INVALID_UTF8 = "(invalid utf8)";
 
-	wchar_t wline[_MAX_LENGTH_ERROR];
+	wchar_t wline[MAX_LENGTH_ERROR];
 	size_t len = 0;
 
 	const char *read = line;
 	const char *error = STR_TOO_LONG;
-	while(len < _MAX_LENGTH)
+	while(len < MAX_LENGTH)
 	{
 		// Read a character. This also advances the read pointer
 		int glyph = str_utf8_decode(&read);
@@ -213,23 +217,23 @@ static void logger_win_console(const char *line, void *user)
 			if(character == 0)
 				break;
 
-			dbg_assert(len < _MAX_LENGTH_ERROR, "str too short for error");
-			wline[len++] = character;
+			dbg_assert(len < MAX_LENGTH_ERROR, "str too short for error");
+			wline[len++] = (unsigned char)character;
 			read++;
 		}
 	}
 
 	// Terminate the line
-	dbg_assert(len < _MAX_LENGTH_ERROR, "str too short for \\r");
+	dbg_assert(len < MAX_LENGTH_ERROR, "str too short for \\r");
 	wline[len++] = '\r';
-	dbg_assert(len < _MAX_LENGTH_ERROR, "str too short for \\n");
+	dbg_assert(len < MAX_LENGTH_ERROR, "str too short for \\n");
 	wline[len++] = '\n';
 
 	// Ignore any error that might occur
 	WriteConsoleW(GetStdHandle(STD_OUTPUT_HANDLE), wline, len, 0, 0);
 
-	#undef _MAX_LENGTH
-	#undef _MAX_LENGTH_ERROR
+	#undef MAX_LENGTH
+	#undef MAX_LENGTH_ERROR
 }
 #endif
 
@@ -2638,7 +2642,7 @@ char* str_sanitize_filename(char* aName)
 	{
 		// replace forbidden characters with a whispace
 		if(*str == '/' || *str == '<' || *str == '>' || *str == ':' || *str == '"'
-			|| *str == '/' || *str == '\\' || *str == '|' || *str == '?' || *str == '*')
+			|| *str == '\\' || *str == '|' || *str == '?' || *str == '*')
  			*str = ' ';
 		str++;
 	}
@@ -3136,7 +3140,7 @@ int str_utf8_decode(const char **ptr)
 	{
 		if((*buf&0x80) == 0x0)  /* 0xxxxxxx */
 		{
-			ch = *buf;
+			ch = (unsigned char)*buf;
 			buf++;
 		}
 		else if((*buf&0xE0) == 0xC0) /* 110xxxxx */

src/engine/client/graphics_threaded.cpp

@@ -418,7 +418,18 @@ int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int Sto
 		return 0;
 	}
 
-	if(Png.depth != 8 || (Png.color_type != PNG_TRUECOLOR && Png.color_type != PNG_TRUECOLOR_ALPHA) || Png.width > (2<<12) || Png.height > (2<<12))
+	if(Png.depth != 8 || Png.width > (2<<12) || Png.height > (2<<12))
+	{
+		dbg_msg("game/png", "invalid format. filename='%s'", aCompleteFilename);
+		io_close(File);
+		return 0;
+	}
+
+	if(Png.color_type == PNG_TRUECOLOR)
+		pImg->m_Format = CImageInfo::FORMAT_RGB;
+	else if(Png.color_type == PNG_TRUECOLOR_ALPHA)
+		pImg->m_Format = CImageInfo::FORMAT_RGBA;
+	else
 	{
 		dbg_msg("game/png", "invalid format. filename='%s'", aCompleteFilename);
 		io_close(File);
@@ -431,10 +442,6 @@ int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int Sto
 
 	pImg->m_Width = Png.width;
 	pImg->m_Height = Png.height;
-	if(Png.color_type == PNG_TRUECOLOR)
-		pImg->m_Format = CImageInfo::FORMAT_RGB;
-	else if(Png.color_type == PNG_TRUECOLOR_ALPHA)
-		pImg->m_Format = CImageInfo::FORMAT_RGBA;
 	pImg->m_pData = pBuffer;
 	return 1;
 }

src/engine/client/serverbrowser.cpp

@@ -466,14 +466,14 @@ CServerEntry *CServerBrowser::Add(int ServerlistType, const NETADDR &Addr)
 		{
 			// alloc start size
 			m_aServerlist[ServerlistType].m_NumServerCapacity = 1000;
-			m_aServerlist[ServerlistType].m_ppServerlist = (CServerEntry **)mem_alloc(m_aServerlist[ServerlistType].m_NumServerCapacity*sizeof(CServerEntry*));
+			m_aServerlist[ServerlistType].m_ppServerlist = (CServerEntry **)mem_alloc(m_aServerlist[ServerlistType].m_NumServerCapacity*sizeof(CServerEntry*)); // NOLINT(bugprone-sizeof-expression)
 		}
 		else
 		{
 			// increase size
 			m_aServerlist[ServerlistType].m_NumServerCapacity += 100;
-			CServerEntry **ppNewlist = (CServerEntry **)mem_alloc(m_aServerlist[ServerlistType].m_NumServerCapacity*sizeof(CServerEntry*));
-			mem_copy(ppNewlist, m_aServerlist[ServerlistType].m_ppServerlist, m_aServerlist[ServerlistType].m_NumServers*sizeof(CServerEntry*));
+			CServerEntry **ppNewlist = (CServerEntry **)mem_alloc(m_aServerlist[ServerlistType].m_NumServerCapacity*sizeof(CServerEntry*)); // NOLINT(bugprone-sizeof-expression)
+			mem_copy(ppNewlist, m_aServerlist[ServerlistType].m_ppServerlist, m_aServerlist[ServerlistType].m_NumServers*sizeof(CServerEntry*)); // NOLINT(bugprone-sizeof-expression)
 			mem_free(m_aServerlist[ServerlistType].m_ppServerlist);
 			m_aServerlist[ServerlistType].m_ppServerlist = ppNewlist;
 		}

src/engine/client/textrender.cpp

@@ -482,6 +482,7 @@ CGlyph *CGlyphMap::GetGlyph(int Chr, int FontSizeIndex, bool Render)
 			m_Glyphs.add(Index);
 			return Index.m_pGlyph;
 		}
+		delete Index.m_pGlyph;
 		return NULL;
 	}
 
@@ -1032,10 +1033,6 @@ void CTextRender::TextNewline(CTextCursor *pCursor)
 	}
 
 	vec2 ScreenScale = vec2(ScreenWidth/(ScreenX1-ScreenX0), ScreenHeight/(ScreenY1-ScreenY0));
-	float Size = pCursor->m_FontSize;
-	int PixelSize = (int)(Size * ScreenScale.y);
-	Size = PixelSize / ScreenScale.y;
-
 	pCursor->m_LineCount++;
 	pCursor->m_Advance.y = pCursor->m_LineSpacing + pCursor->m_NextLineAdvanceY;
 	pCursor->m_Advance.x = 0;

src/engine/external/.clang-tidy

@@ -0,0 +1,3 @@
+# Need at least one check, otherwise clang-tidy fails, use one that can't
+# happen in our code since we don't use OpenMP API.
+Checks: '-*,openmp-exception-escape'

src/game/client/animstate.cpp

@@ -51,7 +51,9 @@ static void AnimSeqEval(CAnimSequence *pSeq, float Time, CAnimKeyframe *pFrame)
 
 static void AnimAddKeyframe(CAnimKeyframe *pSeq, CAnimKeyframe *pAdded, float Amount)
 {
-	pSeq->m_X += pAdded->m_X*Amount;
+	// AnimSeqEval fills m_X for any case, clang-analyzer assumes going into the
+	// final else branch with pSeq->m_NumFrames < 2, which is impossible.
+	pSeq->m_X += pAdded->m_X*Amount; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult)
 	pSeq->m_Y += pAdded->m_Y*Amount;
 	pSeq->m_Angle += pAdded->m_Angle*Amount;
 }

src/game/client/components/broadcast.cpp

@@ -143,8 +143,13 @@ void CBroadcast::OnBroadcastMessage(const CNetMsg_Sv_Broadcast *pMsg)
 		return;
 
 	// new broadcast message
-	int MsgLength = str_length(pMsg->m_pMessage);
+	const int MsgLength = str_length(pMsg->m_pMessage);
 	m_ServerBroadcastReceivedTime = Client()->LocalTime();
+	if(!MsgLength)
+	{
+		m_ServerBroadcastCursor.Reset();
+		return;
+	}
 
 	char aBuf[MAX_BROADCAST_MSG_SIZE];
 	vec4 SegmentColors[MAX_BROADCAST_MSG_SIZE];
@@ -154,7 +159,7 @@ void CBroadcast::OnBroadcastMessage(const CNetMsg_Sv_Broadcast *pMsg)
 	int UserLineCount = 1;
 
 	// parse colors and newline
-	for(int i = 0; i < MsgLength && ServerMsgLen < MAX_BROADCAST_MSG_SIZE - 1; i++)
+	for(int i = 0; i < MsgLength && ServerMsgLen < MAX_BROADCAST_MSG_SIZE - 1 && m_NumSegments < MAX_BROADCAST_MSG_SIZE - 1; i++)
 	{
 		const char *c = pMsg->m_pMessage + i;
 
@@ -264,7 +269,8 @@ void CBroadcast::OnBroadcastMessage(const CNetMsg_Sv_Broadcast *pMsg)
 			m_aServerBroadcastSegments[i].m_IsHighContrast = false;
 		}
 		m_aServerBroadcastSegments[i].m_GlyphPos = m_ServerBroadcastCursor.GlyphCount();
-		TextRender()->TextDeferred(&m_ServerBroadcastCursor, aBuf + SegmentIndices[i], SegmentIndices[i+1] - SegmentIndices[i]);
+		// The segment array always contains exactly m_NumSegments + 1 valid segments but clang-analyzer can't determine that.
+		TextRender()->TextDeferred(&m_ServerBroadcastCursor, aBuf + SegmentIndices[i], SegmentIndices[i+1] - SegmentIndices[i]); // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult)
 	}
 	m_aServerBroadcastSegments[m_NumSegments].m_GlyphPos = m_ServerBroadcastCursor.GlyphCount();
 	TextRender()->TextColor(OldColor);