Zero Length Strings

AKA the Killtarget bug. This one came up during Map Jam 7, and it’s a bug which affects classic Quake and Quoth alike (note to future readers, if Quoth is past version 2.2 then this is no longer the case…). It manifests itself when a rogue entity deletes everything on the map without a targetname – including the player! This is obviously a showstopper, but to understand why it happens we need to explore a subtle detail of the QuakeC language.

Part of what’s confusing about this bug it that the code seems to guard against it already. Today’s code is found in SUB_UseTargets in subs.qc.

if (self.killtarget)
{
...//remove entities that match my killtarget
}

This code translates as “if I have a killtarget, then remove the matching entities”. So how can this code allow anything to remove entities which have no targetname? It’s a quirk of how strings work in QuakeC, and so we’ll start with an example without the quirk: floats.

Load up client.qc and scroll to the bottom of PutClientInServer. We are going to add a bunch of test code which logs information to the console using dprint. By adding it to this function we know it’ll run as soon as we start the mod, and as long as we set “developer 1” in console we can read it. Big dump of code:

	//spawn an entity, with no values set
	spot = spawn();
	
	//we intentionally set a value for count, but leave cnt unset
	spot.count = 0;
	
	//output value of count
	dprint("spot.count='");
	dprint(ftos(spot.count));
	dprint("'\n");
	
	//output value of cnt
	dprint("spot.cnt='");
	dprint(ftos(spot.cnt));
	dprint("'\n\n");
	
	//test if they are equal
	if(spot.count == spot.cnt)
		dprint("spot.cnt equal to spot.count\n\n");
	else
		dprint("spot.cnt not equal to spot.count\n\n");
	
	//test if count is false
	if(spot.count)
		dprint("if(spot.count) follows the TRUE branch\n");
	else
		dprint("if(spot.count) follows the FALSE branch\n");
	
	//test if cnt is false
	if(spot.cnt)
		dprint("if(spot.cnt) follows the TRUE branch\n");
	else
		dprint("if(spot.cnt) follows the FALSE branch\n");

Run this and you should get the following output.

What have we proved? That if you take an clean entity with all fields uninitialised, then setting a field to equal 0 is the same as leaving it unset. The unset field also holds the value 0, the two fields compare equal, and both are treated as being false when used in an if statement. So far, so good – this is what we would expect.

Now we can do the same, but with two string fields: target and targetname. We will leave target as an unset string – one which has never had a value assigned to it. We will set targetname to a zero-length string – a string which outputs no characters when printed.

	//spawn an entity, with no values set
	spot = spawn();
	
	//we intentionally set a value for targetname, but leave target unset
	spot.targetname = "";

	// to emphasise, at this point spot.targetname is a zero-length string, while
	// spot.target is an unset string
	
	//output value of target
	dprint("spot.target='");
	dprint(spot.target);
	dprint("'\n");
	
	//output value of targetname
	dprint("spot.targetname='");
	dprint(spot.targetname);
	dprint("'\n\n");
	
	//test if they are equal
	if(spot.targetname == spot.target)
		dprint("spot.targetname equal to spot.target\n\n");
	else
		dprint("spot.targetname not equal to spot.target\n\n");
	
	//test if target is false
	if(spot.target)
		dprint("if(spot.target) follows the TRUE branch\n");
	else
		dprint("if(spot.target) follows the FALSE branch\n");
	
	//test if targetname is false
	if(spot.targetname)
		dprint("if(spot.targetname) follows the TRUE branch\n");
	else
		dprint("if(spot.targetname) follows the FALSE branch\n");

This time the output is:

Notice on the last line how if(spot.targetname) has evaluated to TRUE! If you set any value on a string, even an zero-length string with no characters, it still counts as a true value. Part of the danger is that a mapper can do this to their entities, by adding the key to an entity without typing any characters into the value. So you can’t simply avoid this error by checking the string assignments within your QuakeC code.

What we can do is change the test. We noticed that the unset string and the empty string compare equal to one another. So if we change the last pair of tests to…

	//test if target is empty
	if(spot.target != "")
		dprint("if(spot.target != "") follows the TRUE branch\n");
	else
		dprint("if(spot.target != "") follows the FALSE branch\n");
	
	//test if targetname is empty
	if(spot.targetname != "")
		dprint("if(spot.targetname != "") follows the TRUE branch\n");
	else
		dprint("if(spot.targetname != "") follows the FALSE branch\n");

…you will notice that the FALSE branch is taken for both fields. This is the fix we need to apply to the problem line in SUB_UseTargets – change it to if (self.killtarget != ""). This is probably the longest article I’ve written for a 4 character bug fix. You should probably do the same for the code involving self.target as well – it’s not as game-breaking to activate all things with no targetname set, since most entities don’t respond to it. However, it can cause unwanted behaviour, so it’s worth a look as well.

In closing, you rarely want to distinguish between unset and zero-length strings, so you should take away the habit of always writing tests for strings with values in form if(string != "") in all future code.

Advertisements