Free Professional Assistance with finding bugs in the source

[FizWizz]

Defects Overview
Number of defects 1,826
Defect Density: 7.84
Header File Problems Overview
Number of defects 3,800
Defect Density: 16.32
Low-Level Interface Problems Overview
Number of defects 1,149
Defect Density: 4.93
Security Vulnerabilities Overview
Number of defects 20
Defect Density: .09
Unused Overview
Number of defects 29,981
Defect Density: 128.76

What would unused lines(?) be? They sure do take the cake.

[patmo98]

What would unused lines(?) be? They sure do take the cake.

Meaby they are lines that are #ifdefed out</pure speculation>

[Flare]

Interesting. I really enjoyed staring at the cluster... I didn't realize so much of Spring was held together that tightly hehe...

Yeah it's so fluffy!
Springs going to get all fixed :D

[el_muchacho]

Hi Zaphod

These are static analysis tools, so no they don't require the code to run. They do need to be able to "build" the code though, in order to analyse them.


Regards

Robert Finking

Hi,

I've had a demo at work on our source code of such a static analysis tool, and I must say the result was extremely impressive. I always thought it would be nice to have it passed on Spring as they do it for free on several major open source softwares (the linux kernel for example).
However we couldn't afford it.
But on Spring, there are a few remaining very hard-to-catch bugs, and hopefully, your tool can spot them.

[Dragon45]

Whoa! Robert, you uploaded the results and forgot to tell us!

Coverity, eh... =-o



Thanks a bundle mate!


BTW, are there more such tool vendors' results incoming, or is the Coverity analysis the only one?

Edit:
Summary (from the file):

Code: Select all

1 	CHECKED_RETURN 	c:\coverity\taspring_r1083\rts\lib\7zip\7zin.c 	SzReadArchiveProperties 	BUG
2 	DEADCODE 	c:\coverity\taspring_r1083\rts\sim\units\cob\cobinstance.cpp 	CCobInstance::Spin(int, int, int, int) 	BUG
3 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\game\ui\gui\guiendgamedialog.cpp 	GUIendgameDialog::ShowPlayerStats() 	BUG
4 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\rendering\map\bfgrounddrawer.cpp 	CBFGroundDrawer::SetHeightTexture() 	BUG
5 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\rendering\map\bfgrounddrawer.cpp 	CBFGroundDrawer::SetExtraTexture(unsigned char *, unsigned char *, bool) 	BUG
6 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\rendering\map\bfgrounddrawer.cpp 	CBFGroundDrawer::SetPathMapTexture() 	BUG
7 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\rendering\map\bfgrounddrawer.cpp 	CBFGroundDrawer::ToggleLosTexture() 	BUG
8 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\lib\7zip\7zextract.c 	SzExtract 	BUG
9 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\lib\7zip\7zextract.c 	SzExtract 	BUG
10 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\sim\units\unitloader.cpp 	CUnitLoader::LoadUnit(std::basic_string<char, std::char_traits<char>, std::allocator<char>>&, float3, int, bool) 	BUG
11 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\sim\units\unithandler.cpp 	CUnitHandler::TestBuildSquare(const float3 &, const UnitDef *, CFeature *&) 	BUG
12 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\sim\units\unitloader.cpp 	CUnitLoader::LoadWeapon(WeaponDef *, CUnit *, UnitDef::UnitDefWeapon *) 	BUG
13 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\sim\movetypes\airmovetype.cpp 	CAirMoveType::UpdateFlying(float, float) 	BUG
14 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\lib\7zip\7zin.c 	SzReadSubStreamsInfo 	BUG
15 	FORWARD_NULL 	c:\coverity\taspring_r1083\rts\game\game.cpp 	CGame::HandleChatMsg(std::basic_string<char, std::char_traits<char>, std::allocator<char>>, int) 	BUG
16 	INVALIDATE_ITERATOR 	c:\coverity\taspring_r1083\rts\system\filesystem\archivescanner.cpp 	CArchiveScanner::WriteCacheData() 	FALSE
17 	NEGATIVE_RETURNS 	c:\coverity\taspring_r1083\rts\game\ui\gui\guiminimap.cpp 	GUIminimap::MouseMoveAction(int, int, int, int, int) 	BUG
18 	RESOURCE_LEAK 	c:\coverity\taspring_r1083\rts\externalai\grouphandler.cpp 	CGroupHandler::TestDll(std::basic_string<char, std::char_traits<char>, std::allocator<char>>) 	BUG
19 	REVERSE_INULL 	c:\coverity\taspring_r1083\rts\game\ui\gui\guigame.cpp 	GUIgame::PrivateDraw() 	BUG
20 	UNINIT 	c:\coverity\taspring_r1083\rts\rendering\textures\texturehandler.cpp 	CTextureHandler::CTextureHandler() [internal] 	BUG
21 	UNINIT 	c:\coverity\taspring_r1083\rts\rendering\textures\texturehandler.cpp 	CTextureHandler::CTextureHandler() [internal] 	BUG
22 	UNUSED_VALUE 	c:\coverity\taspring_r1083\rts\sim\units\cob\cobinstance.cpp 	CCobInstance::Explode(int, int) 	BUG

[Robert.Finking]

Whoa! Robert, you uploaded the results and forgot to tell us!

Hi There,

Thanks for posting this. I started uploading the files to FU yesterday, but then discovered I couldn't access the main Spring site for some reason. Anyway, here are the links to all the results I've had so far. There may be a couple more in the pipeline, though one of the vendors is very reticent to demo their tool this way, so will probably not submit results. In alphabetical order:

Coverity
Klocwork
Programming Research

I hope this is useful to you. The Coverity results are the most concise (they pride themselves on not reporting false positives), and are probably a good starting point. IIRC Coverity reckon average quality code has around one defect per 1000 lines, whereas you've got around 1 defect per 10 000 lines, so your debugging process is clearly working! I'm not sure how much that's affected by the particular settings they've used for this analysis though as the other tools have produced rather more voluminous results.

I'm interested in any thoughts you have on the various results.

Happy Coding

Robert Finking

[SJ]

Hm the coverity report contains false positives I would say. For example the id=10 one says that

At conditional (5): "(ud)->canmove != 0" taking true path
At conditional (6): "(ud)->canfly == 0" taking true path
At conditional (7): "std::operator !=<char, std::char_traits<char>, std::allocator<char>>(std::basic_string<T1, T2, T3>&, const T1 *) != 0" taking true path

and then

Event var_deref_op: Variable "(ud)->movedata" tracked as NULL was dereferenced

But this cant hold true, if the unit is not a factory and has canmove and doesnt fly it must have a movedata (even if the movementclass field is errenous it will get a default moveclass).

The same seems to apply about the extraTex ones in bfgrounddrawer. Havent looked at the rest.

[Robert.Finking]

...But this cant hold true, if the unit is not a factory and has canmove and doesnt fly it must have a movedata (even if the movementclass field is errenous it will get a default moveclass).

Thanks for this. It's odd. If it had messed up this, then you'd also expect it to get the else if(ud->canfly) clause wrong, since that also accesses ud->movedata (line 221). The fact that it hasn't complained about the "canfly" clause makes me think it must have found a real possibilty of a NULL pointer in the case it has complained about - of course you know the code and I don't!

However regardless of whether there is currently a possible "NULL pointer dereference" path right now, if somebody had picked this up in a code review instead of a tool checking it, I'd want to have a defensive non-null check in there anyway, at least as an ASSERT. In my experience, the only time you want to ommit such checks is if you are in the most deeply nested part of some performance critical code. The rest of the time, it's much better to be safe than sorry.

As an aside, ASSERTs are a great tool - they speed up debugging and increase your code robustness tremendously because bugs you didn't even know about start showing up all over the place.... and of course you can switch them off if they're affecting performance. We often use a three-way assert macro which is either switched "off" (pre-processor removes it from the source), "fatal" (default behaviour - standard abort with error message and core dump behaviour) or "log" (write error message to log file, but don't abort). The "log" flavour is obviously slower in the event that messages do need logging, but it does give you a way of gathering ASSERT failure information without annoying your beta-testers by bombing out more often than you would otherwise. You do of course need a way to get the ASSERT logs sent back to you from your beta test team however.

Something like this (I haven't compiled the below so it probably has errors, but you get the idea):

Code: Select all

/**
 * Use this macro to test conditions whose failure indicates a bug
 */
#ifdef ASSERTS_OFF
    #define ASSERT(x)
#else
#ifdef ASSERTS_LOG
    #define ASSERT(x) \
    do \
    { \
        if (!(x)) \
        { \
            AssertLogger::instance().logEvent( \
            "%s:%d:Assertion failed : %s", __FILE__, __LINE__, #x); \
        }\
    } \
    while(false)
#else
    #define ASSERT(x) \
    do \
    { \
        if (!(x)) \
        { \
            fprintf(stderr, \
            "%s:%d:Assertion failed : %s", __FILE__, __LINE__, #x); \
            abort(); \
        }\
    } \
    while(false)
#endif
#endif

I hope this is useful.

Thanks again for you feedback - it's invaluable to have the opinion of the coding team.


Regards

Robert Finking

EDIT: I'm currently comparing output from the various tools and note that Klocwork also picks this one up, though it only indicates it as a possibility rather than a definite:

Code: Select all

189	Critical	1	NUL.GEN.MIGHT	Error	LoadUnit	The dereferenced pointer 'ud->movedata' might be null. Dereferenced object 'ud->movedata' can contain null on line 189 where null value comes from check in condition ud->movedata is false at line 145 	New	Analyze	Defects/Null Pointer Dereference

QAC++ gives no indication of a NULL pointer dereference here however, on the other hand it is concerned about the fact that the LoadWeapon method further down the file checks for weapondef being NULL, but then allows it to be dereferenced anyway. Klocwork is the only tool which picks both of these up. Sorry, I'm thinking "aloud" a bit here - I guess you don't really care about which tool picks up what

Code: Select all

328	Critical	1	NUL.GEN.MIGHT	Error	LoadWeapon	The dereferenced pointer 'weapondef' might be null. Dereferenced object 'weapondef' can contain null on line 328 where null value comes from check in condition weapondef is false at line 322 	New	Analyze	Defects/Null Pointer Dereference

[SJ]

UnitDefHandler contains

ud.movedata=0;
if(ud.canmove && !ud.canfly && ud.type!="Factory"){
string moveclass=tdfparser.SGetValueDef("", "UNITINFO\\MovementClass");
ud.movedata=moveinfo->GetMoveDataFromName(moveclass);

which clearly makes it non null for the case checked. As for checking for null here it would probably just crash at some of the other places that assumes this to be non null for ground units, as for why these doesnt show up in the analyzis I dont know.

[Robert.Finking]

Hi SJ,

Thanks. I wondered if these tools would be able to resolve stuff like this. I wonder if it's unable to correctly reason about public attributes? Normally we encapsulate pretty much everything (again, unless there's a major speed problem), which may help the tools work out the relationships between different methods in a class. I'll have to produce some test cases to check this out.

Cheers

Robert Finking

[AF]

I havent seen anything dealing with dll's be it unitsync.dll the groupAI's or the globalAI's......

[Robert.Finking]

I havent seen anything dealing with dll's be it unitsync.dll the groupAI's or the globalAI's......

Thanks for that observation. That bothers me because I was hoping to see how the different tool coped with the plugin architecture you've got there, but none of the vendors seem to have even tried to tackle it. Of course it's not great news for you guys either - sorry 'bout that!

I'll be sending responses back to the vendors next week at some point so I'll try and remember to quiz them on this.

Regards

Robert Finking

[AF]

Well it they do run the stuff through the groupAI's and JCAI/NTAI I'll be able to see how well as I've fixed numerous bugs in NTAI and seen possible bugs in the groupAI's....

[el_muchacho]

The same seems to apply about the extraTex ones in bfgrounddrawer. Havent looked at the rest.

Is anybody correcting the bugs found by the tools ?

If not, I'll set myself the task of doing it.
Starting with Coverity (I am surprised that it has found so few bugs).
Which numbers have been corrected so far ? For some reason, the berlios.de site seems down at the time I'm writing.

Here are my current findings (based on 0.70b3 downloadable source, I'll update this post until I have a SVN client setup).

These are the easiest ones...
Don't hesitate to point me any blunder I may have made.

2. lines 470-472 are dead code and can be removed

4,5,6,7. bfGroundDrawer.cpp line 1044 should add a test on extraTex != 0 (SJ, in the current state, I think it is possible to dereference a null extraTex)

12. lines 326-460 should be in an else block (to check).

13. replace line 628 "if(lastColWarningType==2 && frontdir.dot(lastColWarning..." with "if(lastColWarningType==2 && lastColWarning && frontdir.dot(lastColWarning..."

15. ??? I guess the intended purpose was to crash the game, in which case lines 2316-2320 should be between #ifdef DEBUG compilation options or something (and never make it to release).

16.

Code: Select all

 		// First delete all outdated information
348  		for (map<string, ArchiveInfo>::iterator i = archiveInfo.begin(); i != archiveInfo.end(); ) {
349  			map<string, ArchiveInfo>::iterator next = i;
350  			next++;
351  			if (!i->second.updated) {
352  				archiveInfo.erase(i);
353  			}
354  			i = next;
355  		}

should be replaced with :

Code: Select all

// First delete all outdated information
for (map<string, ArchiveInfo>::iterator i = archiveInfo.begin(); i != archiveInfo.end(); ) {
	if (!i->second.updated){
		i = archiveInfo.erase(i); 
	}
	else {
		++i;
	}
}

19. lines 390-393 should be moved at the beginning of the following "if (unitdef){...}" block.

20, 21: indeed foundx and foundy are uninitialized. But I've no clue as what initial value to give them (0 ?).

22. line can be commented

[el_muchacho]

Update:

4,5,6,7. I corrected the line number where the correction must be made: 1044, not 1257

12. has been corrected like I suggested, but I finally think it's not a good idea, as what happens if we are in the error case ? Well, the returned address is undefined, and the game will crash anyway sooner or later, but it will be even more difficult to find the cause of the crash.
So how is this case supposed to be handled ? Isn't it a case where the game should simply quit with an error message ?

[Robert.Finking]

Well it they do run the stuff through the groupAI's and JCAI/NTAI I'll be able to see how well as I've fixed numerous bugs in NTAI and seen possible bugs in the groupAI's....

Aqui tiene, voila, etc.

http://www.fileuniverse.com/?p=download ... ip&ID=3153

Don't forget this is based on version 1083 from subversion - line numbers may have changed since then.

I hope this is useful =)

Cheers

Robert Finking

[jcnossen]

That's just a report of JCAI though, not of AF's AI (NTAI).
It would be nice if you could give a report for NTAI as well.

It reports something weird though:
InfoMap.cpp;147;40
FindDefenseBuildingSector
'bestscore' might be used uninitialized in this function. ;;New;Analyze;Defects/Use of Uninitialized Data

Which is not true, due to C++ lazy evaluation it will always first check bestsector.x < 0 (it is initialized with -1), then set the bestscore, and only after that bestsector.x will be >0 and bestscore will be compared.

Code: Select all

int2 InfoMap::FindDefenseBuildingSector (float3 sp, int maxdist)
{
	int2 bestsector(-1,0);
	float bestscore;
	SearchOffset *tbl = GetSearchOffsetTable ();
	int2 sector = GetMapInfoCoords (sp);

	for (int a=0;a<numSearchOffsets;a++)
	{
		SearchOffset& ofs = tbl [a];
		if (ofs.qdist >= maxdist*maxdist)
			break;

		int x = ofs.dx + sector.x;
		int y = ofs.dy + sector.y;
		if (x >= mapw || y >= maph || x < 0 || y < 0)
			continue;
		MapInfo& blk = mapinfo [y*mapw+x];
		if (blk.midh < 0.0f)
			continue;
		if (blk.buildstate)
			continue;

		// higher score means better sector
		GameInfo &gi = gameinfo [y/2*gsw+x/2];
		float score = 1.0f / (1.0f + gi.defense) / (1.0f + sqrtf(ofs.qdist));

		if (bestsector.x < 0 || bestscore < score)
		{
			bestscore = score;
			bestsector = int2 (x,y);
		}
	}
	return bestsector;
}

All in all, I know there are worse bugs in JCAI which it didn't find. These bugs happen after running the AI for a while, and probably have something to do with certain references not being cleared correctly.
So I'm a bit dissapointed with this klocwork tool, it's probably nice for server code where security is a bigger issue, but it's kinda useless here IMO.

[AF]

I'm not sure if any bugs it find will apply to XE8 if it ran through XE7.5, XE8's had a lot of changes, the only agents still intact are the chaser and scouter classes which have had a good few modifications.

That and XE8 seems to be 100% stable as far as I am aware (I've had a month and a half of extensive testing on my rig, aswell as several others, lindirs, arghs, who've been testign extensively).

Should XE8 code be needed I'll be posting it tonight.

Release due at 7:00PM GMT oday (Friday 12th May)

[Robert.Finking]

That's just a report of JCAI though, not of AF's AI (NTAI).
It would be nice if you could give a report for NTAI as well.

Hi There,

Thanks for the further feedback. Agreed, it looks like it's forgotten about the short-curcuit boolean operators (I assume that's what you mean by "lazy"). I don't think any of the vendors will be analysing any more code though. Klocwork did that extra bit as a special request. From our point of view we have all the info we need now, so unless you fancy e-mailing them direct and asking them to take you on as a full time project, you're unlikely to get any further updates. Actually, now I come to think of it Coverity have a list of projects which they continually run through their checker, you might find it easier to get on their list if you're interested: http://scan.coverity.com/ (see also http://news.zdnet.co.uk/software/applic ... 434,00.htm)

I noticed somebody had started picking up the Coverity stuff, but nobody seems to have looked at the Klocwork errors reported on the main body of code? I was just curious how it panned out there? It looked pretty good from the reports I'd checked. Certainly the application we have in mind is not security critical, it's the finding of basic defects like uninitialised data that we're after.

Any how thanks again for your involvement - hopefully it's been useful for you guys too!

Cheers

Robert Finking

[el_muchacho]

Hi,

I don't know if my findings have been incorporated in the last release, as I haven't checked lately (in fact, I don't even have a properly working development environment by now, so I can't test it :/). But I'm pretty sure my analyses are correct and worth looking at, as the reported bugs indeed are bugs, and some of them have been around for a while now.

As for Clokwork, I looked at the problem reports, and they are pretty similar to Coverity.