[doothedew]

I used valgrind to test for memory leaks in this method and this is what I get for the output of valgrind:

LEAK SUMMARY:
==24214== definitely lost: 339 bytes in 5 blocks
==24214== indirectly lost: 0 bytes in 0 blocks
==24214== possibly lost: 80 bytes in 1 blocks
==24214== still reachable: 58 bytes in 1 blocks
==24214== suppressed: 0 bytes in 0 blocks

The rest of the valgrind output is pasted below my code.

How do I fix this method so it doesn't leak memory? Also, why can't I call delete on pBaseURL without seg faulting?

char* URLResolver::resolveURL(string baseURL, string relativeURL)
{
	pBaseURL=new char[baseURL.size()+ relativeURL.size()+ 1]; //create a pointer to point at the base URL
	strcpy(pBaseURL, baseURL.c_str()); //Point the pointer to the baseURL
	pRelativeURL=new char[strlen(relativeURL.c_str()) + 1]; //If there is follow the same pattern as the base
	strcpy(pRelativeURL, relativeURL.c_str());
	char colon = ':';
	char pound ='#';

	//check to make sure url ends with a /
	char*end =strrchr(pBaseURL,'/');
	end--;
	end--;

	if(*end==colon)
	{
		strcat(pBaseURL,"/");
	}
	if(*pRelativeURL==pound)
	{
		// DO NOTHING BECUASE IT HAS ALREADY PARSED THE PAGE
		//strcat(pBaseURL,pRelativeURL);
		//cout<<pBaseURL<<endl;
		return pBaseURL;
	}
	if(baseURL.at(0)=='&')
	{
		return pBaseURL;
	}
	else if(*pRelativeURL=='/') //get only the base directory
	{
		char *tempobj = pBaseURL;
		for (int i=0; i < 3 ;i++)
		{
			pBaseURL++;
			while(*pBaseURL!='/')
			{
				pBaseURL++;
			}
		}
		*pBaseURL=NULL;
		strcat(tempobj,pRelativeURL);
		return tempobj;
	}
	char* checkForPound = strrchr(pRelativeURL,'#');
	if (checkForPound!=NULL)
	{
		*checkForPound=NULL;
	}
	char * pRelativeElement; //create a pointer to point to the elements in the relative url
	int numRelArgs; //int for recording how many directories to go back
	numRelArgs=0;
	char tempString[strlen(pRelativeURL)]; //create a temporary string to hold relative URL elements
	pRelativeElement = strtok (pRelativeURL,"/");
	bool doOnce=true;
	while (pRelativeElement != NULL)
	{
		char * pp = "..";
		char period = '.';
		char * p = &period;
		if(strncmp(pRelativeElement,pp,2)==0)
		{
			numRelArgs++; //count the number of directories needed to go up
		}
		else if(strncmp(pRelativeElement,p,1)!=0)
		{
			if(doOnce)
			{
				strcpy(tempString, pRelativeElement);
				strcat(tempString,"/");
				doOnce=false;
			}
			else
			{
				strcat(tempString, pRelativeElement);
			}
		}
		pRelativeElement = strtok (NULL, "/");
	}
	char * endofFile =strrchr(tempString,'/');
	endofFile++;
	if(*endofFile==NULL)
	{
		endofFile--;
		*endofFile=NULL;
	}
	delete [] tempString;
	for(int i=0;i<numRelArgs+1;i++)
	{
		char * deleteThis;
		deleteThis = strrchr(pBaseURL,'/');
		*deleteThis=NULL;
	}
	strcat(pBaseURL,"/");
	//cout<< strcat(pBaseURL,tempString)<<endl;
	char * retVal= pBaseURL;
	return retVal;
}
URLResolver::~URLResolver()
{
	//delete [] pBaseURL;  Memory Leak? If I try and delete this I get a seg fault
	pBaseURL = NULL;
	delete [] pRelativeURL;
	pRelativeURL=NULL;
}

The rest of the valgrind output:

==24214== 
==24214== HEAP SUMMARY:
==24214==     in use at exit: 477 bytes in 7 blocks
==24214==   total heap usage: 29 allocs, 22 frees, 1,723 bytes allocated
==24214== 
==24214== 54 bytes in 1 blocks are definitely lost in loss record 1 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805427C: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:24)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x805695F: tests() (main.cpp:51)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 55 bytes in 1 blocks are definitely lost in loss record 2 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805427C: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:24)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x8056954: tests() (main.cpp:50)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 58 bytes in 1 blocks are still reachable in loss record 3 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805427C: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:24)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x805696A: tests() (main.cpp:52)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 76 bytes in 1 blocks are definitely lost in loss record 4 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805423B: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:22)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x805695F: tests() (main.cpp:51)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 77 bytes in 1 blocks are definitely lost in loss record 5 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805423B: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:22)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x8056949: tests() (main.cpp:49)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 77 bytes in 1 blocks are definitely lost in loss record 6 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805423B: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:22)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x8056954: tests() (main.cpp:50)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== 80 bytes in 1 blocks are possibly lost in loss record 7 of 7
==24214==    at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
==24214==    by 0x805423B: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:22)
==24214==    by 0x8056670: checkResolved(std::string&) (main.cpp:29)
==24214==    by 0x805696A: tests() (main.cpp:52)
==24214==    by 0x8056BC5: main (main.cpp:70)
==24214== 
==24214== LEAK SUMMARY:
==24214==    definitely lost: 339 bytes in 5 blocks
==24214==    indirectly lost: 0 bytes in 0 blocks
==24214==      possibly lost: 80 bytes in 1 blocks
==24214==    still reachable: 58 bytes in 1 blocks
==24214==         suppressed: 0 bytes in 0 blocks
==24214== 
==24214== For counts of detected and suppressed errors, rerun with: -v
==24214== ERROR SUMMARY: 6 errors from 6 contexts 

[Hiram]

It would help if we knew which line was line 22

A random guess:

pBaseURL=new char[baseURL.size()+ relativeURL.size()+ 1]; //create a pointer to point at the base URL
strcpy(pBaseURL, baseURL.c_str()); //Point the pointer to the baseURL

Say baseURL.size() is 15, and relativeURL.size() is 10. This means the length of pBaseURL is going to be 26, with the last character presumably for \0.

When you perform strcpy, to copy baseURL.c_str() to pBaseURL, you're adding the terminating null (\0), as stated on the strcpy page of the C++ Reference:

"Copies the C string pointed by source into the array pointed by destination, including the terminating null character."

strcpy documentation

So for my example, you'd be losing 10 bytes? I hope that makes sense. You'd want to look into memcpy I believe.

[Salem_c]

Read the messages.

06 ==24214== 54 bytes in 1 blocks are definitely lost in loss record 1 of 7
07 ==24214== at 0x4025F53: operator new[](unsigned int) (vg_replace_malloc.c:299)
08 ==24214== by 0x805427C: URLResolver::resolveURL(std::string, std::string) (URLResolver.cpp:24)
09 ==24214== by 0x8056670: checkResolved(std::string&) (main.cpp:29)
10 ==24214== by 0x805695F: tests() (main.cpp:51)
11 ==24214== by 0x8056BC5: main (main.cpp:70)

This file:line is where you're calling new (we can't tell from here, since your snippet lines don't match your real code).
Now follow that pointer until it goes out of scope, and then add delete [] ptr; just before it does go out of scope.

Rinse and repeat for all the other leaks.

[baavgai]

The first question would be, why are you using c-strings. Followed by, why does this require a class.

I suppose I'd start out with something like:

URLResolver::URLResolver() : pBaseURL(NULL), pRelativeURL(NULL) { }

URLResolver::~URLResolver() { clear(); }

void URLResolver::clear() {
	if (pBaseURL != NULL) { 
		delete [] pBaseURL;
		pBaseURL = NULL;
	}
	if (pRelativeURL != NULL) { 
		delete [] pRelativeURL;
		pRelativeURL = NULL;
	}
}

char* URLResolver::resolveURL(string baseURL, string relativeURL) {
	clear();
//...

[Bench]

doothedew, on 22 July 2011 - 06:13 AM, said:

How do I fix this method so it doesn't leak memory?

As a general rule of thumb, avoid using new in C++ code unless you really have to. You could fix all of your memory-leak problems by using string (or maybe vector<char>) instead, and you'llend up with a much cleaner, safer solution which is easier to maintain.

doothedew, on 22 July 2011 - 06:13 AM, said:

char* URLResolver::resolveURL(string baseURL, string relativeURL)
{
	pBaseURL=new char[baseURL.size()+ relativeURL.size()+ 1]; 

Here you've allocated a chunk of memory, but was it pointing to something else before?

doothedew, on 22 July 2011 - 06:13 AM, said:

	if(*pRelativeURL==pound)
	{
		// DO NOTHING BECUASE IT HAS ALREADY PARSED THE PAGE
		//strcat(pBaseURL,pRelativeURL);
		//cout<<pBaseURL<<endl;
		return pBaseURL; 

And here you're leaving it up to whoever calls the function to delete it.
This should immediately raise alarm bells; its a memory leak just waiting to happen, since functions shouldn't rely on their calling code to clean up after them. Memory allocation inside a function which is not cleaned up inside that function is a very bad side-effect, and really needs to be avoided.

doothedew, on 22 July 2011 - 06:13 AM, said:

	if(baseURL.at(0)=='&')
	{
		return pBaseURL; 

And here

doothedew, on 22 July 2011 - 06:13 AM, said:

		char *tempobj = pBaseURL;
		for (int i=0; i < 3 ;i++)
		{
			pBaseURL++;
			while(*pBaseURL!='/')
			{
				pBaseURL++;
			}
		}
		*pBaseURL=NULL;
		strcat(tempobj,pRelativeURL);
		return tempobj; 

And here too

doothedew, on 22 July 2011 - 06:13 AM, said:

URLResolver::~URLResolver()
{
	//delete [] pBaseURL;  Memory Leak? If I try and delete this I get a seg fault
	pBaseURL = NULL;
	delete [] pRelativeURL;
	pRelativeURL=NULL;
}

Its possible that you've ended up with a dangling pointer because you called delete on pBaseUrl without setting it to a NULL value afterwards (its safe to call delete NULL;)

Everything you're doing in your function can easily be done with string. If, for whatever reason, you don't wish to use 'string', you can treat a vector<char> just like a char* by grabbing the address-of its first element. i.e. &my_vector[0]

This post has been edited by Bench: 23 July 2011 - 02:15 AM