- Fix a bunch of off-by one errors in irr::core::string in functions...

- Fix a bunch of off-by one errors in irr::core::string in functions equals_substring_ignore_case, findFirst, findFirstChar, findNext, findLast, findLastChar, replace, remove and removeChars. This prevents some potential memory access errors, find functions no longer try to find the \0, replace no longer replaces the \0 and remove no longer tries to remove it (which did remove the last character instead).
- Fix a few new warnings in gcc.



git-svn-id: svn://svn.code.sf.net/p/irrlicht/code/trunk@4308 dfc29bdd-3216-0410-991c-e03cc46cb475

changes.txt

 Changes in 1.8 (??.??.2011)
 
+ - Fix a bunch of off-by one errors in irr::core::string in functions equals_substring_ignore_case, findFirst, findFirstChar, findNext, findLast, findLastChar, replace, remove and removeChars.
+   This prevents some potential memory access errors, find functions no longer try to find the \0, replace no longer replaces the \0 and remove no longer tries to remove it (which did remove the last character instead).
+
  - matrix4::setRotationAxisRadians added
 
  - quaternion conversions to and from matrix4 no longer invert rotations. 

include/irrString.h

@@ -501,7 +501,7 @@ public:
 	//! Makes the string lower case.
 	string<T,TAlloc>& make_lower()
 	{
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; array[i]; ++i)
 			array[i] = locale_lower ( array[i] );
 		return *this;
 	}
@@ -510,7 +510,7 @@ public:
 	//! Makes the string upper case.
 	string<T,TAlloc>& make_upper()
 	{
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; array[i]; ++i)
 			array[i] = locale_upper ( array[i] );
 		return *this;
 	}
@@ -534,7 +534,7 @@ public:
 	\return True if the strings are equal ignoring case. */
 	bool equals_substring_ignore_case(const string<T,TAlloc>&other, const s32 sourcePos = 0 ) const
 	{
-		if ( (u32) sourcePos > used )
+		if ( (u32) sourcePos >= used )
 			return false;
 
 		u32 i;
@@ -717,7 +717,7 @@ public:
 	or -1 if not found. */
 	s32 findFirst(T c) const
 	{
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 			if (array[i] == c)
 				return i;
 
@@ -736,7 +736,7 @@ public:
 		if (!c || !count)
 			return -1;
 
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 			for (u32 j=0; j<count; ++j)
 				if (array[i] == c[j])
 					return i;
@@ -806,7 +806,7 @@ public:
 	or -1 if not found. */
 	s32 findNext(T c, u32 startPos) const
 	{
-		for (u32 i=startPos; i<used; ++i)
+		for (u32 i=startPos; i<used-1; ++i)
 			if (array[i] == c)
 				return i;
 
@@ -821,7 +821,7 @@ public:
 	or -1 if not found. */
 	s32 findLast(T c, s32 start = -1) const
 	{
-		start = core::clamp ( start < 0 ? (s32)(used) - 1 : start, 0, (s32)(used) - 1 );
+		start = core::clamp ( start < 0 ? (s32)(used) - 2 : start, 0, (s32)(used) - 2 );
 		for (s32 i=start; i>=0; --i)
 			if (array[i] == c)
 				return i;
@@ -841,7 +841,7 @@ public:
 		if (!c || !count)
 			return -1;
 
-		for (s32 i=used-1; i>=0; --i)
+		for (s32 i=(s32)used-2; i>=0; --i)
 			for (u32 j=0; j<count; ++j)
 				if (array[i] == c[j])
 					return i;
@@ -1006,7 +1006,7 @@ public:
 	\param replaceWith Character replacing the old one. */
 	string<T,TAlloc>& replace(T toReplace, T replaceWith)
 	{
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 			if (array[i] == toReplace)
 				array[i] = replaceWith;
 		return *this;
@@ -1128,7 +1128,7 @@ public:
 	{
 		u32 pos = 0;
 		u32 found = 0;
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 		{
 			if (array[i] == c)
 			{
@@ -1153,7 +1153,7 @@ public:
 			return *this;
 		u32 pos = 0;
 		u32 found = 0;
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 		{
 			u32 j = 0;
 			while (j < size)
@@ -1186,7 +1186,7 @@ public:
 
 		u32 pos = 0;
 		u32 found = 0;
-		for (u32 i=0; i<used; ++i)
+		for (u32 i=0; i<used-1; ++i)
 		{
 			// Don't use characters.findFirst as it finds the \0,
 			// causing used to become incorrect.

source/Irrlicht/CColladaMeshWriter.cpp

@@ -143,14 +143,14 @@ bool CColladaMeshWriterProperties::useNodeMaterial(const scene::ISceneNode* node
 		return false;
 
 	// TODO: we need some ISceneNode::hasType() function to get rid of those checks
-	bool useMeshMaterial =	(	(node->getType() == ESNT_MESH ||	
+	bool useMeshMaterial =	(	(node->getType() == ESNT_MESH ||
 								node->getType() == ESNT_CUBE ||
 								node->getType() == ESNT_SPHERE ||
 								node->getType() == ESNT_WATER_SURFACE ||
 								node->getType() == ESNT_Q3SHADER_SCENE_NODE)
 								&& static_cast<const IMeshSceneNode*>(node)->isReadOnlyMaterials())
 
-							||	(node->getType() == ESNT_ANIMATED_MESH 
+							||	(node->getType() == ESNT_ANIMATED_MESH
 								&& static_cast<const IAnimatedMeshSceneNode*>(node)->isReadOnlyMaterials() );
 
 	return !useMeshMaterial;
@@ -180,7 +180,7 @@ irr::core::stringw CColladaMeshWriterNames::nameForNode(const scene::ISceneNode*
 	irr::core::stringw name;
 	// Prefix, because xs::ID can't start with a number, also nicer name
 	if ( node && node->getType() == ESNT_LIGHT )
-		name = L"light";	
+		name = L"light";
 	else
 		name = L"node";
 	name += nameForPtr(node);
@@ -368,7 +368,7 @@ bool CColladaMeshWriter::writeScene(io::IWriteFile* file, scene::ISceneNode* roo
 		if ( root->getType() != ESNT_SCENE_MANAGER )
 		{
 			// TODO: Not certain if we should really write the root or if we should just always only write the children.
-			// For now writing root to keep backward compatibility for this case, but if anyone needs to _not_ write 
+			// For now writing root to keep backward compatibility for this case, but if anyone needs to _not_ write
 			// that root-node we can add a parameter for this later on in writeScene.
 			writeSceneNode(root);
 		}
@@ -378,7 +378,7 @@ bool CColladaMeshWriter::writeScene(io::IWriteFile* file, scene::ISceneNode* roo
 			// so we do not write the root itself if it points to the scenemanager.
 			const core::list<ISceneNode*>& rootChildren = root->getChildren();
 			for ( core::list<ISceneNode*>::ConstIterator it = rootChildren.begin();
-					it != rootChildren.end(); 
+					it != rootChildren.end();
 					++ it )
 			{
 				writeSceneNode(*it);
@@ -444,7 +444,7 @@ void CColladaMeshWriter::writeNodeMaterials(irr::scene::ISceneNode * node)
 	IMesh* mesh = getProperties()->getMesh(node);
 	if ( mesh )
 	{
-		MeshNode * n = Meshes.find(mesh);	
+		MeshNode * n = Meshes.find(mesh);
 		if ( !getProperties()->useNodeMaterial(node) )
 		{
 			// no material overrides - write mesh materials
@@ -452,7 +452,7 @@ void CColladaMeshWriter::writeNodeMaterials(irr::scene::ISceneNode * node)
 			{
 				writeMeshMaterials(mesh, getGeometryWriting() == ECGI_PER_MESH_AND_MATERIAL ? &materialNames : NULL);
 				n->getValue().MaterialsWritten = true;
-			}		
+			}
 		}
 		else
 		{
@@ -617,6 +617,8 @@ void CColladaMeshWriter::writeNodeLights(irr::scene::ISceneNode * node)
 				Writer->writeClosingTag(L"directional");
 				Writer->writeLineBreak();
 				break;
+			default:
+				break;
 		}
 
 		Writer->writeClosingTag(L"technique_common");
@@ -684,7 +686,7 @@ void CColladaMeshWriter::writeNodeCameras(irr::scene::ISceneNode * node)
 
 		Writer->writeClosingTag(L"technique_common");
 		Writer->writeLineBreak();
-	
+
 		Writer->writeClosingTag(L"optics");
 		Writer->writeLineBreak();
 
@@ -732,7 +734,7 @@ void CColladaMeshWriter::writeSceneNode(irr::scene::ISceneNode * node )
 	Writer->writeLineBreak();
 
 	// DummyTransformationSceneNode don't have rotation, position, scale information
-	// But also don't always export the transformation matrix as that forces us creating 
+	// But also don't always export the transformation matrix as that forces us creating
 	// new DummyTransformationSceneNode's on import.
 	if ( node->getType() == ESNT_DUMMY_TRANSFORMATION )
 	{
@@ -740,7 +742,7 @@ void CColladaMeshWriter::writeSceneNode(irr::scene::ISceneNode * node )
 	}
 	else
 	{
-		irr::core::vector3df rot(node->getRotation()); 
+		irr::core::vector3df rot(node->getRotation());
 		if ( isCamera(node) && !static_cast<ICameraSceneNode*>(node)->getTargetAndRotationBinding() )
 		{
 			ICameraSceneNode * camNode = static_cast<ICameraSceneNode*>(node);
@@ -748,10 +750,10 @@ void CColladaMeshWriter::writeSceneNode(irr::scene::ISceneNode * node )
 			rot = toTarget.getHorizontalAngle();
 		}
 
-		writeTranslateElement( node->getPosition() ); 
-		writeRotateElement( irr::core::vector3df(1.f, 0.f, 0.f), rot.X ); 	 
-		writeRotateElement( irr::core::vector3df(0.f, 1.f, 0.f), rot.Y ); 	 
-		writeRotateElement( irr::core::vector3df(0.f, 0.f, 1.f), rot.Z ); 	 
+		writeTranslateElement( node->getPosition() );
+		writeRotateElement( irr::core::vector3df(1.f, 0.f, 0.f), rot.X );
+		writeRotateElement( irr::core::vector3df(0.f, 1.f, 0.f), rot.Y );
+		writeRotateElement( irr::core::vector3df(0.f, 0.f, 1.f), rot.Z );
 		writeScaleElement( node->getScale() );
 	}
 
@@ -1528,7 +1530,7 @@ void CColladaMeshWriter::writeMeshGeometry(const irr::core::stringw& meshname, s
 
 	// write texture coordinates
 
-	core::stringw meshTexCoord0Id(meshId); 
+	core::stringw meshTexCoord0Id(meshId);
 	meshTexCoord0Id += L"-TexCoord0";
 	Writer->writeElement(L"source", false, L"id", meshTexCoord0Id.c_str());
 	Writer->writeLineBreak();
@@ -2226,7 +2228,7 @@ void CColladaMeshWriter::writeMatrixElement(const irr::core::matrix4& matrix)
 			if ( b > 0 )
 				txt += " ";
 			// row-column switched compared to Irrlicht
-			txt += irr::core::stringw(matrix[b*4+a]); 
+			txt += irr::core::stringw(matrix[b*4+a]);
 		}
 		Writer->writeText(txt.c_str());
 		Writer->writeLineBreak();

source/Irrlicht/CLightSceneNode.cpp

@@ -69,6 +69,8 @@ void CLightSceneNode::render()
 						LightData.Direction * LightData.Radius,
 						LightData.DiffuseColor.toSColor());
 				break;
+			default:
+				break;
 		}
 	}
 

source/Irrlicht/COpenGLDriver.cpp

@@ -3448,6 +3448,8 @@ void COpenGLDriver::assignHardwareLight(u32 lightIndex)
 		glLightf(lidx, GL_SPOT_EXPONENT, 0.0f);
 		glLightf(lidx, GL_SPOT_CUTOFF, 180.0f);
 	break;
+	default:
+	break;
 	}
 
 	// set diffuse color

source/Irrlicht/CSoftwareDriver2.cpp

@@ -1942,6 +1942,8 @@ s32 CBurningVideoDriver::addDynamicLight(const SLight& dl)
 			l.linearAttenuation = 1.f / dl.Radius;
 			l.quadraticAttenuation = dl.Attenuation.Z;
 
+			break;
+		default:
 			break;
 	}
 
@@ -2162,6 +2164,8 @@ void CBurningVideoDriver::lightVertex ( s4DVertex *dest, u32 vertexargb )
 				// diffuse component
 				diffuse.mulAdd ( light.DiffuseColor, dot );
 				break;
+			default:
+				break;
 		}
 
 	}
@@ -2228,7 +2232,7 @@ void CBurningVideoDriver::draw2DImage(const video::ITexture* texture, const core
 			os::Printer::log("Fatal Error: Tried to copy from a surface not owned by this driver.", ELL_ERROR);
 			return;
 		}
-	
+
 	if (useAlphaChannelOfTexture)
 		StretchBlit(BLITTER_TEXTURE_ALPHA_BLEND, RenderTargetSurface, &destRect, &sourceRect,
 			    ((CSoftwareTexture2*)texture)->getImage(), (colors ? colors[0].color : 0));
@@ -2237,7 +2241,7 @@ void CBurningVideoDriver::draw2DImage(const video::ITexture* texture, const core
 			    ((CSoftwareTexture2*)texture)->getImage(), (colors ? colors[0].color : 0));
 	}
 }
-    
+
 //! Draws a 2d line.
 void CBurningVideoDriver::draw2DLine(const core::position2d<s32>& start,
 					const core::position2d<s32>& end,

tests/irrString.cpp

@@ -180,6 +180,104 @@ bool testAppendStringc()
 	return true;
 }
 
+bool testLowerUpper()
+{
+	irr::core::array <irr::core::stringc> stringsOrig, targetLower, targetUpper;
+	stringsOrig.push_back("abc");
+	targetLower.push_back("abc");
+	targetUpper.push_back("ABC");
+	stringsOrig.push_back("ABC");
+	targetLower.push_back("abc");
+	targetUpper.push_back("ABC");
+	stringsOrig.push_back("Abc");
+	targetLower.push_back("abc");
+	targetUpper.push_back("ABC");
+	stringsOrig.push_back("aBBc");
+	targetLower.push_back("abbc");
+	targetUpper.push_back("ABBC");
+	stringsOrig.push_back("abC");
+	targetLower.push_back("abc");
+	targetUpper.push_back("ABC");
+	stringsOrig.push_back("");
+	targetLower.push_back("");
+	targetUpper.push_back("");
+	/* TODO: those are not supported so far
+	stringsOrig.push_back("ßäöü");
+	targetLower.push_back("ßäöü");
+	targetUpper.push_back("ßÄÖÜ");
+	stringsOrig.push_back("ßÄÖÜ");
+	targetLower.push_back("ßäöü");
+	targetUpper.push_back("ßÄÖÜ");
+	*/
+
+	for ( irr::u32 i=0; i<stringsOrig.size(); ++i )
+	{
+		irr::core::stringc c = stringsOrig[i];
+		c.make_lower();
+		if ( c != targetLower[i] )
+		{
+			logTestString("make_lower for stringc failed in test %d %s\n", i, stringsOrig[i].c_str());
+			return false;
+		}
+
+		c = stringsOrig[i];
+		c.make_upper();
+		if ( c != targetUpper[i] )
+		{
+			logTestString("make_upper for stringc failed in test %d %s %s\n", i, stringsOrig[i].c_str(), c.c_str());
+			return false;
+		}
+
+		irr::core::stringw w = irr::core::stringw(stringsOrig[i]);
+		c.make_lower();
+		if ( c != irr::core::stringw(targetLower[i]) )
+		{
+			logTestString("make_lower for stringw failed in test %d %s\n", i, stringsOrig[i].c_str());
+			return false;
+		}
+
+		c = irr::core::stringw(stringsOrig[i]);
+		c.make_upper();
+		if ( c != irr::core::stringw(targetUpper[i]) )
+		{
+			logTestString("make_upper for stringw failed in test %d %s\n", i, stringsOrig[i].c_str());
+			return false;
+		}
+	}
+
+	return true;
+}
+
+bool testFindFunctions()
+{
+	irr::core::stringc dot(".");
+	irr::s32 p = dot.findFirst(0);
+	if ( p >= 0 )
+		return false;
+
+	irr::core::stringc empty("");
+	p = empty.findLastCharNotInList("x",1);
+	if ( p >= 0 )
+		return false;
+
+	p = empty.findLast('x');
+	if ( p >= 0 )
+		return false;
+
+	p = dot.findLast('.');
+	if ( p != 0 )
+		return false;
+
+	p = empty.findLastChar("ab", 2);
+	if ( p >= 0 )
+		return false;
+
+	p = dot.findLastChar("-.", 2);
+	if ( p != 0 )
+		return false;
+
+	return true;
+}
 
 // Test the functionality of irrString
 /** Validation is done with assert_log() against expected results. */
@@ -243,6 +341,12 @@ bool testIrrString(void)
 	logTestString("test replace\n");
 	allExpected &= testReplace();
 
+	logTestString("test make_lower and make_uppers\n");
+	allExpected &= testLowerUpper();
+
+	logTestString("test find functions\n");
+	allExpected &= testFindFunctions();
+
 	if(allExpected)
 		logTestString("\nAll tests passed\n");
 	else

tests/tests-last-passed-at.txt

 Tests finished. 1 test of 1 passed.
 Compiled as DEBUG
-Test suite pass at GMT Sat Jul  7 10:28:50 2012
+Test suite pass at GMT Thu Sep  6 19:39:44 2012