Various small fixes identified by cppcheck #1371

dpogue · 2023-04-09T03:32:14Z

Best to review this one commit at a time and I can drop any commits that people deem controversial.

This does not clear up all the issues that cppcheck found, but these were some low-hanging fruit.

dgelessus · 2023-04-09T10:48:55Z

Sources/Plasma/CoreLib/HeadSpin.h

@@ -411,7 +411,7 @@ void DebugMsg(const char* fmt, ...);

 #if defined(_MSC_VER)
 #define  DEFAULT_FATAL(var)  default: FATAL("No valid case for switch variable '" #var "'"); __assume(0); break;
-#elif defined(__GNUC__)
+#elif defined(__GNUC__) || defined(__clang__)


For anyone else wondering: Clang also defines __GNUC__, but only on Unix platforms. On Windows, you need to explicitly check __clang__.

Hmm, is that accurate? I would have expected this to be more of a clang vs clang-cl distinction, not a Unix vs Windows distinction...

Yeah, that's what I would have expected too, but it seems to make no difference. I tested using the Clang 12 from VS 2019, and both clang and clang-cl define only __clang__ and not __GNUC__.

Ok, it might be more of an SDK thing then -- Clang from MSYS2, for example, does define __GNUC__ even on Windows, but that version would be using the MinGW SDK rather than the MSVC Windows SDK. Either way, the fix is appropriate, since Clang doesn't always define __GNUC__... It just piqued my curiosity :)

Sources/Plasma/CoreLib/hsBitVector.cpp

Sources/Plasma/PubUtilLib/plDrawable/plWaveSet7.cpp

Sources/Plasma/PubUtilLib/plGLight/plShadowMaster.cpp

zrax · 2023-04-09T18:23:10Z

Sources/Plasma/CoreLib/HeadSpin.h

@@ -411,7 +411,7 @@ void DebugMsg(const char* fmt, ...);

 #if defined(_MSC_VER)
 #define  DEFAULT_FATAL(var)  default: FATAL("No valid case for switch variable '" #var "'"); __assume(0); break;
-#elif defined(__GNUC__)
+#elif defined(__GNUC__) || defined(__clang__)


Hmm, is that accurate? I would have expected this to be more of a clang vs clang-cl distinction, not a Unix vs Windows distinction...

zrax · 2023-04-09T18:27:40Z

Sources/Plasma/PubUtilLib/plGImage/plFont.cpp

@@ -1706,7 +1706,7 @@ class plBDFCharsParser : public plBDFSectParser
            {
                // If we're doing data, all lines are hex values until we hit "ENDCHAR"
                if( fRowsLeft == 0 )
-                    throw;
+                    throw false;


Hmm, this is suspicious... throw without any parameter will re-throw the current exception or call std::terminate if there isn't one... But what does throwing a bool accomplish? Who's catching from this call? I feel like either changing this to throw a proper exception (like std::runtime_error) or an explicit crash (like hsAssert(false)) would be better.

There are several other throw statements in this function that were throwing bools, so this was just making it consistent, but I'll try to investigate whether anything is actually catching these errors. Maybe hsAssert is a better option.

So it appears that calls to parse the BDF contents are wrapped in a try/catch block, and catch both std::exception and non-exceptions:

Plasma/Sources/Plasma/PubUtilLib/plGImage/plFont.cpp

Lines 2100 to 2112 in 5b47a75

catch (std::exception &e)

{

printf("Exception caught in plFont::LoadFromBDF: %s\n", e.what());

IClear();

fclose(fp);

return false;

}

catch (...)

{

IClear();

fclose( fp );

return false;

}

It seems the only place this BDF code is actually used is in plFontConverter, and freetype2 has built-in BDF support so it's unclear to me if this code is still serving any purpose or if it should be removed.

It looks like this should probably throw a real std::exception for logging purposes.

dpogue · 2023-04-09T20:43:56Z

Sources/Plasma/PubUtilLib/plGLight/plShadowMaster.cpp

                break;
        }
-        s = 1 << i;
+        hsAssert(i >= 0, "Invalid non-POT shadow size");
+        s = 1u << i;


cppcheck is still warning about i potentially being uint32_t(-1), but I think that's because hsAssert doesn't guarantee end of execution when HS_DEBUGGING is set.

Sources/Plasma/PubUtilLib/plDrawable/plWaveSet7.cpp

Sources/Plasma/PubUtilLib/plAvatar/plAvBrainCoop.cpp

Hoikas · 2023-04-13T01:39:58Z

Sources/Plasma/PubUtilLib/plAvatar/plArmatureMod.cpp

@@ -859,6 +859,11 @@ void plArmatureMod::SpawnAt(int spawnNum, double time)

 void plArmatureMod::SetFollowerParticleSystemSO(plSceneObject *follower)
 {
+    if (!follower) {
+        fFollowerParticleSystemSO = follower;


This doesn't seem to set up the key reference for the new follower (see line 873), which could cause the follower to be thrown away from under us.

In this case, follower is null

I'm still concerned about this. While it is true that follower is null, this seems like it's leaking the reference established on line 873. I do think it would be better to make it more obvious that fFollowerParticleSystemSO is being set to nullptr.

It looks like there is a case (albeit perhaps unused in normal gameplay) where this method gets called with nullptr, and in theory the current code will crash.

If your preference to use the existing fFollowerParticleSystemSO to remove the reference (and maybe change SetAttachedToAvatar(false) on the particle system itself?) before setting it to null?

I believe that's correct. We'll basically want to send another plAttachMsg with plRefMsg::kOnRemove, I think.

I've updated this to send a detach message

For future reference, I went through and verified that sending a plAttachMsg with the context kOnRemove will release the key reference. This happens in plCoordinateInterface::IDetachChild().

Hoikas · 2023-10-15T21:28:13Z

What is the status here?

dpogue · 2023-10-16T02:21:30Z

I think this was just waiting for re-review

In theory there's no way this would ever really cause issues, but it's easy enough to resolve by shuffling ifdefs around.

Pretty sure the intention was to filter out non-attach ref messages, but the default case was allowing them through before the ref message check ever ran.

Co-Authored-By: dgelessus <[email protected]>

There's one spot that does actually appear to call this with nullptr, so I'm not sure how this hasn't been causing crashes... Co-Authored-By: Adam Johnson <[email protected]>

Co-Authored-By: dgelessus <[email protected]>

We're already asserting that the size of the containers will fit in a uint16_t, but this was getting flagged by CodeQL as a potential overflow.

Co-Authored-By: dgelessus <[email protected]>

dgelessus reviewed Apr 9, 2023

View reviewed changes

zrax reviewed Apr 9, 2023

View reviewed changes

dpogue force-pushed the cppcheck branch from c5fa200 to 5ae03f3 Compare April 9, 2023 20:41

dpogue commented Apr 9, 2023

View reviewed changes

dpogue force-pushed the cppcheck branch from 5ae03f3 to 9d3d61a Compare April 9, 2023 20:53

zrax reviewed Apr 9, 2023

View reviewed changes

Sources/Plasma/PubUtilLib/plDrawable/plWaveSet7.cpp Outdated Show resolved Hide resolved

dpogue force-pushed the cppcheck branch from 9d3d61a to 0261a13 Compare April 9, 2023 22:21

dgelessus reviewed Apr 10, 2023

View reviewed changes

Sources/Plasma/PubUtilLib/plAvatar/plAvBrainCoop.cpp Outdated Show resolved Hide resolved

dpogue force-pushed the cppcheck branch 3 times, most recently from 498f96f to e8949ee Compare April 11, 2023 03:47

dgelessus approved these changes Apr 11, 2023

View reviewed changes

Hoikas reviewed Apr 13, 2023

View reviewed changes

Deledrius approved these changes Oct 19, 2023

View reviewed changes

dpogue and others added 14 commits October 23, 2023 20:11

Fix some missing va_end calls found by cppcheck

fe60c89

Fix multiple definitions of plAVIWriter

e52777c

In theory there's no way this would ever really cause issues, but it's easy enough to resolve by shuffling ifdefs around.

Clang supports __builtin_unreacheable

b1561ea

Fix potential access violation in avatar brains

0b2b5d8

Fix unreachable check in NetMsg screener

d27b72a

Pretty sure the intention was to filter out non-attach ref messages, but the default case was allowing them through before the ref message check ever ran.

Fix invalid throw statement

c5713f0

Fix potential overflow iterating an hsBitVector

41cc375

Co-Authored-By: dgelessus <[email protected]>

Catch null dereference with avatar particle systems

1fef9f9

There's one spot that does actually appear to call this with nullptr, so I'm not sure how this hasn't been causing crashes... Co-Authored-By: Adam Johnson <[email protected]>

Type the fixed locations enum as uint32

2a83e8b

Ensure variable is initialized before passing reference

4b0c1cd

Use unsigned ints for waveset graphs

53731af

Co-Authored-By: dgelessus <[email protected]>

Catch hypothetical negative shift in shadow master

ff257da

Co-Authored-By: dgelessus <[email protected]>

Explicitly cast size_t to uint16_t in a few loops

a197480

We're already asserting that the size of the containers will fit in a uint16_t, but this was getting flagged by CodeQL as a potential overflow.

Fix hypothetical uint multiplication overflow

c8592c6

Fix plAvBrainCoop printf format specifier

3db7cdb

Co-Authored-By: dgelessus <[email protected]>

dpogue force-pushed the cppcheck branch from e8949ee to 3db7cdb Compare October 24, 2023 03:47

Hoikas approved these changes Oct 28, 2023

View reviewed changes

Hoikas merged commit 1c4209c into H-uru:master Nov 3, 2023

dpogue deleted the cppcheck branch November 3, 2023 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various small fixes identified by cppcheck #1371

Various small fixes identified by cppcheck #1371

dpogue commented Apr 9, 2023

dgelessus Apr 9, 2023

zrax Apr 9, 2023

dgelessus Apr 9, 2023

zrax Apr 9, 2023 •

edited

Loading

zrax Apr 9, 2023

zrax Apr 9, 2023

dpogue Apr 9, 2023

dpogue Apr 11, 2023

Hoikas Oct 16, 2023

dpogue Apr 9, 2023

Hoikas Apr 13, 2023

dpogue Apr 13, 2023

Hoikas Oct 16, 2023

dpogue Oct 17, 2023

Hoikas Oct 17, 2023

dpogue Oct 24, 2023

Hoikas Oct 25, 2023

Hoikas commented Oct 15, 2023

dpogue commented Oct 16, 2023

	catch (std::exception &e)
	{
	printf("Exception caught in plFont::LoadFromBDF: %s\n", e.what());
	IClear();
	fclose(fp);
	return false;
	}
	catch (...)
	{
	IClear();
	fclose( fp );
	return false;
	}

Various small fixes identified by cppcheck #1371

Various small fixes identified by cppcheck #1371

Conversation

dpogue commented Apr 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrax Apr 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoikas commented Oct 15, 2023

dpogue commented Oct 16, 2023

zrax Apr 9, 2023 •

edited

Loading