Malloc fails...why?

damir_tk

I don't understand why this fails: LPTSTR FileHelper::GetFileFullPath( LPWSTR lpwName ) { LPTSTR lptFileFullPath; DWORD len = GetCurrentDirectory( 0, NULL ); LPWSTR lpwFolder; lpwFolder = (LPWSTR) malloc( len ); if ( lpwFolder == NULL ) { free ( lpwFolder ); return NULL; } if ( GetCurrentDirectory( len, lpwFolder ) != 0 ) { len = (DWORD) ( wcslen( lpwFolder ) + wcslen( lpwName ) + 6 ); lptFileFullPath = (LPTSTR) malloc( len ); // Here lptFileFullPath simply evaluates to NULL, showing the ?! if ( lptFileFullPath == NULL ) { free ( lptFileFullPath ); return NULL; } // ...the rest of the code stripped off... } return lptFileFullPath; } This is under Visual Studio 2005, UNICODE defined by default. Thanks.

Taka Muraoka

I think your first malloc is wrong. "len" is the length of the current directory in characters but malloc() takes a number of bytes i.e. it should be sizeof(TCHAR) * len.

0 bottles of beer on the wall, 0 bottles of beer, you take 1 down, pass it around, 4294967295 bottles of beer on the wall. Awasu 2.2.3 [^]: A free RSS/Atom feed reader with support for Code Project.

Waldermort

First, why are you not using new and delete? and second, why are you calling free() on what you already know to be a NULL pointer? Now for your problem, GetCurrentDirectory() is returning the length required excluding the terminating NULL character, but you are not adding this when you call malloc. so your second call will read the string and not write the NULL. Later when you call wstrlen(), it is reading the string uo until the first NULL it finds, which could be anywhere. Therefore may return an insanly high value. When used in malloc, it returns NULL because there is not enough memory. The problem has nothing to do with sizeof(TCHAR) ;P

Mark Salsbery

I would recommend looking VERY closely at all the string functions you use. Some use/return lengths in characters. Some use/return lengths in bytes. Some include NULL terminator in lengths. Some do not. LPTSTR FileHelper::GetFileFullPath( LPWSTR lpwName ) { LPTSTR lptFileFullPath; DWORD LenInChars = GetCurrentDirectory( 0, NULL ); ++LenInChars; // add 1 for NULL terminator LPWSTR lpwFolder; lpwFolder = (LPWSTR) malloc( LenInChars * sizeof(TCHAR) ); if ( lpwFolder == NULL ) { return NULL; // or throw new CIScrewedUpException(); } if ( GetCurrentDirectory( LenInChars, lpwFolder ) != 0 ) { // added 1 for NULL terminator LenInChars = (DWORD) ( wcslen( lpwFolder ) + wcslen( lpwName ) + 1 ); lptFileFullPath = (LPTSTR) malloc( LenInChars * sizeof(TCHAR)); // Here lptFileFullPath simply evaluates to NULL, showing the ?! if ( lptFileFullPath == NULL ) { // prevent memory leak free( lpwFolder ); return NULL; } // ...the rest of the code stripped off... } // prevent memory leak free( lpwFolder ); return lptFileFullPath; } *EDIT* And for goodness sakes, use new/delete instead of malloc/free!!! :)

damir_tk

Thanks, you are totally right.

damir_tk

Haha...okay, I like your style (both of writing and of coding), and thanks for your help. Now 3 questions: 1. Why malloc sucks, I don't see any problems with it. New and delete is better when you are creating some objects that have a constructor and a destructor, as they get called automatically, but not if you use malloc. But here I only use strings and string pointers, so why? 2. I already rewrite the code, to use something like: lpwFolder = (LPWSTR) malloc( (len + 1) * 2 ); Is it wrong? (Apart from not being elegant :=) ). 3. I use _tcslen instead of wcslen, is it better suited here? Thanks for your help again.

Mark Salsbery

damir_tk wrote:

1. Why malloc sucks, I don't see any problems with it. New and delete is better when you are creating some objects that have a constructor and a destructor, as they get called automatically, but not if you use malloc. But here I only use strings and string pointers, so why?

I didn't say it sucks ;) You're using C++ so why not take advantage of the much stronger type safety provided by the language? Using "new", you create a typed pointer so the compiler can help point out mistakes during development because it knows the pointers type without having to use the old C-style cast. Use what you want, of course, but I bet you'll find new and delete much more pleasing to use.

damir_tk wrote:

2. I already rewrite the code, to use something like: lpwFolder = (LPWSTR) malloc( (len + 1) * 2 ); Is it wrong? (Apart from not being elegant :=) ). 3. I use _tcslen instead of wcslen, is it better suited here?

The only thing I see that's not elegant is mixing the generic "T" types with the hard-coded "W" types. By using the "T" types you have the advantage of your code working for both UNICODE and non-UNICODE builds. It also fits the way the Windows APIs are declared. If you know you are only ever going to use UNICODE then you could just use the "W" string types. It's just easier to read the code if you stick to one set of types and functions. This is all just my opinions of course. :) Here's another version of your code using all "T" char types and new/delete...

LPTSTR FileHelper::GetFileFullPath( LPCTSTR lpwName )
{
	LPTSTR lptFileFullPath;
	DWORD LenInChars = ::GetCurrentDirectory( 0, NULL );
	++LenInChars; // add 1 for NULL terminator
	LPTSTR lpwFolder = new TCHAR[ LenInChars ];
	if ( lpwFolder == NULL )
	{
		return NULL;
	}
	if ( ::GetCurrentDirectory( LenInChars, lpwFolder ) != 0 )
	{
		// I added 6 like you did before...you may need more chars for backslashes and such
		LenInChars = (DWORD)( _tcslen( lpwFolder ) + _tcslen( lpwName ) + 6 );
		lptFileFullPath = new TCHAR[ LenInChars ];
		if ( lptFileFullPath == NULL )
		{
			// prevent memory leak
			delete[] lpwFolder;
			return NULL;
		}
		// ...the rest of the code stripped off...
	}
	// prevent memory leak
	delete[] lpwFolder;
	return lptFileFullPath;
}

damir_tk

Thank a lot. Point taken. :)