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);

src/game/client/components/menus.cpp

@@ -382,15 +382,15 @@ void CMenus::DoJoystickBar(const CUIRect *pRect, float Current, float Tolerance,
 int CMenus::DoKeyReader(CButtonContainer *pButtonContainer, const CUIRect *pRect, int Key, int Modifier, int* pNewModifier)
 {
 	// process
-	static const void *s_pGrabbedID = 0;
+	static const void *s_pGrabbedID = 0x0;
 	static bool s_MouseReleased = true;
 	static int s_ButtonUsed = 0;
 
 	const bool Hovered = UI()->MouseHovered(pRect);
 	int NewKey = Key;
 	*pNewModifier = Modifier;
 
-	if(!UI()->MouseButton(0) && !UI()->MouseButton(1) && s_pGrabbedID == pButtonContainer)
+	if(!UI()->MouseButton(0) && !UI()->MouseButton(1) && s_pGrabbedID != 0x0 && s_pGrabbedID == pButtonContainer)
 		s_MouseReleased = true;
 
 	if(UI()->CheckActiveItem(pButtonContainer))

src/game/client/components/scoreboard.cpp

@@ -258,9 +258,9 @@ float CScoreboard::RenderScoreboard(float x, float y, float w, int Team, const c
 	// render title
 	if(NoTitle)
 	{
-		if(m_pClient->m_Snap.m_pGameData->m_GameStateFlags&GAMESTATEFLAG_GAMEOVER)
+		if(Snap.m_pGameData && Snap.m_pGameData->m_GameStateFlags&GAMESTATEFLAG_GAMEOVER)
 			pTitle = Localize("Game over");
-		else if(m_pClient->m_Snap.m_pGameData->m_GameStateFlags&GAMESTATEFLAG_ROUNDOVER)
+		else if(Snap.m_pGameData && Snap.m_pGameData->m_GameStateFlags&GAMESTATEFLAG_ROUNDOVER)
 			pTitle = Localize("Round over");
 		else
 			pTitle = Localize("Scoreboard");

src/game/client/components/stats.cpp

@@ -375,7 +375,6 @@ void CStats::OnRender()
 			s_Cursor.MoveTo(x + 64, y + LineHeight / 2.0f);
 			TextRender()->TextOutlined(&s_Cursor, "⋅⋅⋅ ", -1);
 			TextRender()->TextOutlined(&s_Cursor, aBuf, -1);
-			px += 100;
 			break;
 		}
 
@@ -604,7 +603,6 @@ void CStats::OnRender()
 					TextRender()->TextOutlined(&s_Cursor, aBuf, -1);
 				}
 			}
-			px += 100;
 		}
 		y += LineHeight;
 	}

src/game/client/gameclient.cpp

@@ -670,7 +670,7 @@ void CGameClient::OnMessage(int MsgId, CUnpacker *pUnpacker)
 		if(GameMsgID < 0 || GameMsgID >= NUM_GAMEMSGS)
 			return;
 
-		int aParaI[3];
+		int aParaI[3] = {0, 0, 0};
 		int NumParaI = 0;
 
 		// get paras

src/game/editor/editor.cpp

@@ -1265,9 +1265,9 @@ void CEditor::DoQuadEnvelopes(const array<CQuad> &lQuads, IGraphics::CTextureHan
 {
 	int Num = lQuads.size();
 	CEnvelope **apEnvelope = new CEnvelope*[Num];
-	mem_zero(apEnvelope, sizeof(CEnvelope*)*Num);
 	for(int i = 0; i < Num; i++)
 	{
+		apEnvelope[i] = 0;
 		if((m_ShowEnvelopePreview == SHOWENV_SELECTED && lQuads[i].m_PosEnv == m_SelectedEnvelope) || m_ShowEnvelopePreview == SHOWENV_ALL)
 			if(lQuads[i].m_PosEnv >= 0 && lQuads[i].m_PosEnv < m_Map.m_lEnvelopes.size())
 				apEnvelope[i] = m_Map.m_lEnvelopes[lQuads[i].m_PosEnv];
@@ -2671,7 +2671,7 @@ void CEditor::RenderFileDialog()
 	// GUI coordsys
 	CUIRect View = *UI()->Screen();
 	Graphics()->MapScreen(View.x, View.y, View.w, View.h);
-	CUIRect Preview;
+	CUIRect Preview = {0, 0, 0, 0};
 	float Width = View.w, Height = View.h;
 
 	View.Draw(vec4(0,0,0,0.25f), 0.0f, CUIRect::CORNER_NONE);
@@ -2782,9 +2782,9 @@ void CEditor::RenderFileDialog()
 		}
 	}
 
-	if(m_FilesSelectedIndex >= 0 && m_FilesSelectedIndex < m_FilteredFileList.size())
+	if(m_FilesSelectedIndex >= 0 && m_FilesSelectedIndex < m_FilteredFileList.size() && m_FileDialogFileType == CEditor::FILETYPE_IMG)
 	{
-		if(m_FileDialogFileType == CEditor::FILETYPE_IMG && !m_PreviewImageIsLoaded)
+		if(!m_PreviewImageIsLoaded)
 		{
 			int Length = str_length(m_FilteredFileList[m_FilesSelectedIndex]->m_aFilename);
 			if(Length >= str_length(".png") && str_endswith_nocase(m_FilteredFileList[m_FilesSelectedIndex]->m_aFilename, ".png"))