Fixed potential issue in Common::String when asserting a substring of a string X back to X (memcpy -> memmove); also added some other sanity checks, and merged some duplicate code into a new method String::initWithCStr

svn-id: r33137

common/str.cpp

@@ -43,32 +43,43 @@ static int computeCapacity(int len) {
 	return ((len + 32 - 1) & ~0x1F) - 1;
 }
 
-String::String(const char *str, uint32 len)
+String::String(const char *str) : _len(0), _str(_storage) {
-: _len(0), _str(_storage) {
+	if (str == 0) {
+		_storage[0] = 0;
+		_len = 0;
+	} else
+		initWithCStr(str, strlen(str));
+}
+
+String::String(const char *str, uint32 len) : _len(0), _str(_storage) {
+	initWithCStr(str, len);
+}
+
+String::String(const char *beginP, const char *endP) : _len(0), _str(_storage) {
+	assert(endP >= beginP);
+	initWithCStr(beginP, endP - beginP);
+}
+
+void String::initWithCStr(const char *str, uint32 len) {
+	assert(str);
 
 	// Init _storage member explicitly (ie. without calling its constructor)
 	// for GCC 2.95.x compatibility (see also tracker item #1602879).
 	_storage[0] = 0;
 
-	if (str && *str) {
+	_len = len;
-		const uint32 tmp = strlen(str);
-		assert(len <= tmp);
-		if (len <= 0)
-			len = tmp;
-		_len = len;
 
-		if (len >= _builtinCapacity) {
+	if (len >= _builtinCapacity) {
-			// Not enough internal storage, so allocate more
+		// Not enough internal storage, so allocate more
-			_extern._capacity = computeCapacity(len);
+		_extern._capacity = computeCapacity(len);
-			_extern._refCount = 0;
+		_extern._refCount = 0;
-			_str = (char *)malloc(_extern._capacity+1);
+		_str = (char *)malloc(_extern._capacity+1);
-			assert(_str != 0);
+		assert(_str != 0);
-		}
-
-		// Copy the string into the storage area
-		memcpy(_str, str, len);
-		_str[len] = 0;
 	}
+
+	// Copy the string into the storage area
+	memmove(_str, str, len);
+	_str[len] = 0;
 }
 
 String::String(const String &str)
@@ -91,6 +102,8 @@ String::String(char c)
 	_storage[0] = c;
 	_storage[1] = 0;
 
+	// TODO/FIXME: There is no reason for the following check -- we *do*
+	// allow strings to contain 0 bytes!
 	_len = (c == 0) ? 0 : 1;
 }
 
@@ -130,11 +143,14 @@ String& String::operator  =(const char *str) {
 	uint32 len = strlen(str);
 	ensureCapacity(len, false);
 	_len = len;
-	memcpy(_str, str, len + 1);
+	memmove(_str, str, len + 1);
 	return *this;
 }
 
 String &String::operator  =(const String &str) {
+	if (&str == this)
+		return *this;
+
 	if (str.isStorageIntern()) {
 		decRefCount(_extern._refCount);
 		_len = str._len;

common/str.h

@@ -96,10 +96,24 @@ public:
 	static const char *emptyString;
 #endif
 
+	/** Construct a new empty string. */
 	String() : _len(0), _str(_storage) { _storage[0] = 0; }
-	String(const char *str, uint32 len = 0);
+
+	/** Construct a new string from the given NULL-terminated C string. */
+	String(const char *str);
+
+	/** Construct a new string containing exactly len characters read from address str. */
+	String(const char *str, uint32 len);
+	
+	/** Construct a new string containing the characters between beginP (including) and endP (excluding). */
+	String(const char *beginP, const char *endP);
+	
+	/** Construct a copy of the given string. */
 	String(const String &str);
+	
+	/** Construct a string consisting of the given character. */
 	String(char c);
+
 	~String();
 
 	String &operator  =(const char *str);
@@ -162,7 +176,7 @@ public:
 
 	void toLowercase();
 	void toUppercase();
-
+	
 	uint hash() const;
 
 public:
@@ -189,6 +203,7 @@ protected:
 	void ensureCapacity(uint32 new_len, bool keep_old);
 	void incRefCount() const;
 	void decRefCount(int *oldRefCount);
+	void initWithCStr(const char *str, uint32 len);
 };
 
 // Append two strings to form a new (temp) string

test/common/str.h

@@ -5,16 +5,25 @@
 class StringTestSuite : public CxxTest::TestSuite
 {
 	public:
-	void test_empty_clear( void )
+	void test_constructors(void) {
-	{
+		Common::String str("test-string");
+		TS_ASSERT( str == "test-string" );
+		str = Common::String(str.c_str()+5, 3);
+		TS_ASSERT( str == "str" );
+		str = "test-string";
+		TS_ASSERT( str == "test-string" );
+		str = Common::String(str.c_str()+5, str.c_str()+8);
+		TS_ASSERT( str == "str" );
+	}
+
+	void test_empty_clear(void) {
 		Common::String str("test");
 		TS_ASSERT( !str.empty() );
 		str.clear();
 		TS_ASSERT( str.empty() );
 	}
 
-	void test_lastChar( void )
+	void test_lastChar(void) {
-	{
 		Common::String str;
 		TS_ASSERT_EQUALS( str.lastChar(), '\0' );
 		str = "test";
@@ -23,8 +32,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( str2.lastChar(), 'r' );
 	}
 
-	void test_concat1( void )
+	void test_concat1(void) {
-	{
 		Common::String str("foo");
 		Common::String str2("bar");
 		str += str2;
@@ -32,22 +40,19 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( str2, "bar" );
 	}
 
-	void test_concat2( void )
+	void test_concat2(void) {
-	{
 		Common::String str("foo");
 		str += "bar";
 		TS_ASSERT_EQUALS( str, "foobar" );
 	}
 
-	void test_concat3( void )
+	void test_concat3(void) {
-	{
 		Common::String str("foo");
 		str += 'X';
 		TS_ASSERT_EQUALS( str, "fooX" );
 	}
 
-	void test_refCount( void )
+	void test_refCount(void) {
-	{
 		Common::String foo1("foo");
 		Common::String foo2("foo");
 		Common::String foo3(foo2);
@@ -57,8 +62,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( foo3, "foo""X" );
 	}
 
-	void test_refCount2( void )
+	void test_refCount2(void) {
-	{
 		Common::String foo1("fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd");
 		Common::String foo2("fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd");
 		Common::String foo3(foo2);
@@ -68,8 +72,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( foo3, "fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd""X" );
 	}
 
-	void test_refCount3( void )
+	void test_refCount3(void) {
-	{
 		Common::String foo1("0123456789abcdefghijk");
 		Common::String foo2("0123456789abcdefghijk");
 		Common::String foo3(foo2);
@@ -79,8 +82,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( foo3, "0123456789abcdefghijk""0123456789abcdefghijk" );
 	}
 
-	void test_refCount4( void )
+	void test_refCount4(void) {
-	{
 		Common::String foo1("fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd");
 		Common::String foo2("fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd");
 		Common::String foo3(foo2);
@@ -90,8 +92,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( foo3, "fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd""fooasdkadklasdjklasdjlkasjdlkasjdklasjdlkjasdasd" );
 	}
 
-	void test_hasPrefix( void )
+	void test_hasPrefix(void) {
-	{
 		Common::String str("this/is/a/test, haha");
 		TS_ASSERT_EQUALS( str.hasPrefix(""), true );
 		TS_ASSERT_EQUALS( str.hasPrefix("this"), true );
@@ -99,8 +100,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( str.hasPrefix("foo"), false );
 	}
 
-	void test_hasSuffix( void )
+	void test_hasSuffix(void) {
-	{
 		Common::String str("this/is/a/test, haha");
 		TS_ASSERT_EQUALS( str.hasSuffix(""), true );
 		TS_ASSERT_EQUALS( str.hasSuffix("haha"), true );
@@ -108,8 +108,7 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( str.hasSuffix("hahah"), false );
 	}
 
-	void test_contains( void )
+	void test_contains(void) {
-	{
 		Common::String str("this/is/a/test, haha");
 		TS_ASSERT_EQUALS( str.contains(""), true );
 		TS_ASSERT_EQUALS( str.contains("haha"), true );
@@ -117,15 +116,13 @@ class StringTestSuite : public CxxTest::TestSuite
 		TS_ASSERT_EQUALS( str.contains("test"), true );
 	}
 
-	void test_toLowercase( void )
+	void test_toLowercase(void) {
-	{
 		Common::String str("Test it, NOW! 42");
 		str.toLowercase();
 		TS_ASSERT_EQUALS( str, "test it, now! 42" );
 	}
 
-	void test_toUppercase( void )
+	void test_toUppercase(void) {
-	{
 		Common::String str("Test it, NOW! 42");
 		str.toUppercase();
 		TS_ASSERT_EQUALS( str, "TEST IT, NOW! 42" );