String Class (Memory Leak...?)

[GameDude]

I am working with strings (using them as path names for loading files), and I have the following, really basic line of code:

string temp = "";

That's it. No tricks here.

However, when I execute this in the debug environment, the compiler sets the variable equal to...well...a directory path. Not an empty string. Not even complete junk like normal non-initialized variables have. But an actual string, seen elsewhere in my program, around 10 - 15 characters long.

It's always the same "directory path" string of characters, so at least it's consistant. I've worked with strings for years and haven't seen anything quite like this before. Maybe a memory leak...?

 

[cpjust]

Do you have a small program you can post that reproduces this behavior?

 

[GameDude]

Sort of...

My program is completely object oriented, with thousands of lines of code, and dozens of classes. However, I tried to narrow it down as simply as possible. I can't give you the files it tries to open, but I can at least show you the code and what it's trying to do.

What you're looking for is the beginning of a function "aFunction", toward the bottom. ANY STRINGS it initializes in that function are filled with the root directory of the application.

As far as I have tested, this is every line executed before it fails, and no less. I removed all lines that did not effect the error occuring.

Note: This code references a header file with wrapper classes for loading files and getting hold of the keyboard. These classes have been used in dozens of other applications of mine and have never caused problems. However, if you want them so you can try and run it yourself, let me know.

#define WIN32_LEAN_AND_MEAN

#define STR(y) (LPTSTR)y.c_str()

//include files
#include <windows.h>
#include <stdlib.h>
#include "time.h"
#include <string>
#include "Core.h"

enum State
{
	LEV_LOAD,
	LEV_UPDATE,
	MENU_LOAD,
	MENU_UPDATE
};

using std::string;

int paintCount;
bool running = true;

LRESULT CALLBACK WndProc( HWND hWnd, UINT messg,
								WPARAM wParam, LPARAM lParam );

void aFunction();

int WINAPI WinMain( HINSTANCE hInst, 	/*Win32 entry-point routine */
					HINSTANCE hPreInst, 
					LPSTR lpszCmdLine, 
					int nCmdShow )
{
	#pragma unused( lpszCmdLine )
	
	WNDCLASS wc;
	clsKeyboard keyboard;
	MSG lpMsg;
	HWND hWnd;
	
	//register the window class
	if(!hPreInst)			/*set up window class and register it */
	{
		wc.lpszClassName 	= "Something";
		wc.hInstance 		= hInst;
		wc.lpfnWndProc		= WndProc;
		wc.hCursor			= LoadCursor(NULL, IDC_ARROW);
		wc.hIcon			= LoadIcon(NULL, IDI_APPLICATION);
		wc.lpszMenuName		= NULL;
		wc.hbrBackground	= (HBRUSH)GetStockObject(WHITE_BRUSH);
		wc.style			= 0;
		wc.cbClsExtra		= 0;
		wc.cbWndExtra		= 0;

		if(!RegisterClass(&wc))
		{
			MessageBox(NULL, "Error Registering Window Class", "Error", MB_OK);
			return false;
		}
	}
	
	//load information from files
	LPTSTR tempStr = "";
	string root = "";
	string directory = "";
	string err = "";
	DWORD temp;
	
	//get the root directory
	GetCurrentDirectory(temp, tempStr);
	root = tempStr;
	
	for(int x = 0; x < root.length(); x++)
		if(root[x] == '\\')
			root[x] = '/';
			
	//start loading files
	directory = root + "/Animations/Animations.ini";
	InitFile *initAnim = new InitFile(STR(directory));
	if(initAnim -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	directory = root + "/Objects.ini";
	InitFile *initObj = new InitFile(STR(directory));
	if(initObj -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	directory = root + "/Ships.ini";
	InitFile *initShuttle = new InitFile(STR(directory));
	if(initShuttle -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	directory = root + "/Weapons.ini";
	InitFile *initWeapons = new InitFile(STR(directory));
	if(initWeapons -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	directory = root + "/User.ini";
	InitFile *initUser = new InitFile(STR(directory));
	if(initUser -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	directory = root + "/Levels/Levels.ini";
	InitFile *initLevels = new InitFile(STR(directory));
	if(initUser -> failed())
	{
		err = "Failed to Open " + directory;
		MessageBox(NULL, STR(err), "Error", MB_OK);
		return false;
	}
	
	double totalTime = GetTickCount();
	double currentTick = GetTickCount();
	int elapsedTime = 2000;
	State mode = LEV_LOAD;
	
	while(running)					
	{
		currentTick = GetTickCount();
		aFunction();
		if(keyboard.keyIsDown(VK_ESCAPE))
			running = false;
		else
		{
			if(GetMessage(&lpMsg, NULL, 0, 0))
			{
				TranslateMessage(&lpMsg);
				DispatchMessage(&lpMsg);
			}
			UpdateWindow(hWnd);
		}
		while((GetTickCount() - currentTick) < ((1 / 60) * 100))
			running = running;
		elapsedTime = GetTickCount() - currentTick;
	}
	
	return( lpMsg.wParam );
}

void aFunction()
{
	long currentX = 0;
	long currentY = 0;
	long currentWidth = 0;
	long currentHeight = 0;
	string bkg = "";
	POINT sSize, lSize;
	LPTSTR tempStr = "";
	string root = "";
	string err = "";
	DWORD temp;
	DEVMODE dm;
	
	//get the root directory
	GetCurrentDirectory(temp, tempStr);
	root = tempStr;
	
	//switch out all '/' with '\'
	for(int x = 0; x < root.length(); x++)
		if(root[x] == '\\')
			root[x] = '/';
	string path = "/";
}

LRESULT CALLBACK WndProc( HWND hWnd, UINT messg,				/*callback procedure */
								WPARAM wParam, LPARAM lParam )
{
	switch(messg)
	{
		case WM_PAINT:
		paintCount++;
		break;
		
		case WM_DESTROY:
			PostQuitMessage(0);
			running = false;
		break;
		
		default:
			return( DefWindowProc( hWnd, messg, wParam, lParam ) );
	}

	return(0L);
}

 

[cpjust]

I can't compile the program since I don't have your header file, not to mention the endless number of other errors I get, but I did find this problem in several places:

LPTSTR tempStr = "";
DWORD temp;

GetCurrentDirectory(temp, tempStr);

An LPTSTR is just a pointer (which you are pointing to a literal NULL char), and you don't initialize your DWORD before using it.
GetCurrentDirectory() is therefore being passed a random number and a 1-byte string buffer which is being vastly overflowed when GetCurrentDirectory() writes to it.

I'd be willing to bet that's the problem you're seeing.

 

[GameDude]

So if I changed it to, say, this

LPTSTR tempStr = new char[30];
DWORD temp = 0;

GetCurrentDirectory(temp, tempStr);

then memory wouldn't overflow, and it might solve the problem. I'll try it.

 

[GameDude]

It didn't work. All allocating memory did was fill the string with nonsense (something like "A<*knz") and make GetCurrentDirectory refuse to place anything in it at all.

 

[cpjust]

That's because you told GetCurrentDirectory() that you were passing a 0-byte string.
You should set temp to the size of tempStr.
GetCurrentDirectory() needs to know how big the string buffer is so it doesn't overflow the buffer.

 

[uolj]

Using (LPTSTR)y.c_str() is really a bad idea. The only time it will even work is if you are guaranteed that whoever wrote the code expecting an LPTSTR made a mistake in const correctness and really should have been requesting an LPCTSTR (the C is for const). For example, the InitFile constructor apparently takes an LPTSTR. If it attempts to modify that in any way, you will get very weird results.

Only use c_str() when you need a const char * (LPCSTR or LPCTSTR).

 

[cpjust]

Also, you should be using C++ casting, not C casting.
i.e. static_cast<LPCTSTR>(y.c_str()) instead of (LPCTSTR)y.c_str()

There are 4 types of casts in C++:
const_cast
static_cast
dynamic_cast
reinterpret_cast

I also see a lot of new's in your code, but no delete's. All dynamically allocated memory should always be deleted at some point.

 

[uolj]

Note that casting the result of c_str() to an LPCTSTR is not necessary. Assuming you are in a non-UNICODE environment, c_str() returns a const char* which is exactly what LPCTSTR is a typedef for.

Also, to use a C++ style cast to cast the result of c_str() to LPTSTR, you wouldn't be able to use static_cast, you would have to use a const_cast. In this case it shouldn't matter, because you shouldn't be doing it.

 

[GameDude]

I use the string class to edit strings. I never edit anything using the LPTSTR. However, many of the functions I use require LPTSTR as arguments.

So...

How SHOULD I approach the conversion from a string class to a LPTSTR, since I shouldn't use c_str()?

 

[cpjust]

I'm not sure what InitFile does with the string, but MessageBox() takes an LPCTSTR, so you don't even need to cast it.
If you are doing something like this:

string str = "Hello";
char* pStr = (char*)str.c_str();
strcat( pStr, " World" );

Then that is a big no-no. STL strings keep track of how long their strings are and could use that internal length when you call the str.size() function instead of recalculating the length each time. Since you edited the class's internal string yourself, it didn't update its length variable...
Not to mention it might not have allocated enough room for the string you're trying to write to it.

You could try using a vector<char> like this:

vector<char> vStr;
vStr.reserve( 12 );  // Make sure vector is at least 12 chars long.
strcpy( vStr.front(), "Hello" );
strcat( vStr.front(), " World" );

// Now copy to a string.
string str( vStr.begin(), vStr.end() );

 

[uolj]

If you never edit anything using the LPTSTR, then it should be an LPCTSTR. You still have to be careful, though, because you are using a pointer to the internal memory of a string object, which could easily change depending on what you do with the original string variable.

For example, in your code you keep changing the value of the directory variable. If you are storing the LPTSTR or even if you used LPCTSTR, in your initAnim variable, then it will point to a different string the next time you change directory. Same for all the other places.

I would use a string everywhere I could. In cases where you call functions from outside sources needing a const char*, you can use c_str() for LPCTSTR if the pointer isn't expected to stay the same for very long. If it does have to remain valid and you can't use string, then you would need to copy the value to your own persistent character array.

If the string needs to be modified by an outside function like GetCurrentDirectory, then you can start with a character array or vector<char> and then convert that to a string object after you call it. I'd prefer vector<char>, but here is an example of each. Encapsulating them into functions makes everything cleaner:

std::string GetCurrentDirectoryString()
{
    DWORD pathSize = GetCurrentDirectory(0, 0); // Get length of path.
    std::vector<char> currentDir(pathSize);
    GetCurrentDirectory(pathSize, &currentDir[0]);
    return std::string(&currentDir[0]);
}

std::string GetCurrentDirectoryString()
{
    DWORD pathSize = GetCurrentDirectory(0, 0); // Get length of path.
    char* currentDir = new char[pathSize];
    GetCurrentDirectory(pathSize, currentDir);
    std::string directory(currentDir);
    delete [] currentDir;
    return directory;
}

 

[cpjust]

Oh crap, I just noticed I forgot to put & infront of my calls to vStr.front().
&vStr[0] and &vStr.front() both return a pointer to the first element of the char array.

I'd highly recommend buying "Effective STL: 50 Specific Ways to Improve Your Use of the Standard Template Library"

http://www.amazon.com/Effective-STL..._bbs_sr_3/002-2403453-1429628?ie=UTF8&s=books

as well as "C++ Coding Standards: 101 Rules, Guidelines, and Best Practices"

http://www.amazon.com/C++-Coding-St..._bbs_sr_1/002-2403453-1429628?ie=UTF8&s=books

They're FULL of very useful info about C++ and STL programming.