C Dynamic Memory Allocation Error

[kaftab]

Hi Gurus,

Trying to append a comma to a string. Getting "Segmentation Error" on Solaris when the function is entered the second time.

// Appends a comma to the given string
void appendComma(char* instring)
{
	if (instring == NULL)
	{	
		instring = realloc(NULL, strlen(","));
		strcpy(instring,",");
	}
	else
	{	
		instring = realloc(instring, strlen(instring) + strlen(",")); // ===> Segmentation error here
		strcat(instring,",");
	}		
}

Your help is alwasy appreciated.

[Peter_B]

To hold a single character (e.g. the comma) you need to allocate 2 bytes, not 1, as you are doing. The extra one is for the null terminator - which also needs to be written after the string.

[kaftab]

Thanks for responding.

Why do I need to append the null terminator? This is not the end of the string. There are more characters to be appened to it.

And besides, this shouldn't create a memory error, should it?

Regardless, to try this, I changed the code to:

instring = realloc(instring, strlen(instring) + strlen(",")+1);

but that didn't change the outcome.

[Lindley]

When you call appendComma, a copy of the pointer is passed in. Then you call realloc(), which discards the original memory.

At this point you have a problem. You have stored the new block of memory in the instring parameter, but outside the function you still have a pointer to the old, now invalid memory (or NULL.)

There are two possible fixes. The first, and to my mind the more elegant, is to return the new value of instring rather than returning void.

The second is to make instring be a char** rather than a char*, so that changes to the pointer are reflected outside the function. (In C++, this could be more cleanly done by making instring a char*&....or, better yet, just using a std::string in the first place.)

[Lindley]

Why do I need to append the null terminator? This is not the end of the string. There are more characters to be appened to it.

You have this in your code:
strlen(instring)

This call cannot possibly work if you don't have a proper NULL terminator. For that matter, strcat() won't work either. Now, repeatedly calling strlen() for successive appends of a single character is definitely not the most efficient approach, but if you're going to do it then your string must always be null-terminated, whether it is "complete" or not.

A more efficient scheme, which incidentally does not require NULL termination, would be to pass in the size of the string as a parameter.

If you want to do NULL termination properly, the change is actually quite simple: replace strlen(",") with sizeof(","). This will work since "," is a string literal (do not try it on pointers like instring), and it will return 2 instead of 1.

[kaftab]

I changed the code per your recommendation (if I had the luxury of using C++, I wouldn't be asking these dumb questions ). I still get the error at memory allocation:

// Appends a comma to the given string
char* appendComma(char* inString)
{
	char* outString = NULL;

	if (inString == NULL)
	{	
		outString = malloc(sizeof(","));
		if (outString == NULL)
			return inString;
		strcpy(outString,",");
	}
	else
	{	
		outString = malloc(strlen(inString) + sizeof(","));  // <=== Segmentation error here
		if (outString == NULL)
			return inString;
		strcat(outString,",");
	}		

	return outString;
}

[Lindley]

I don't see any reason to switch from realloc to malloc here. You still need realloc's functionality.

[kaftab]

Why do we need realloc? We are assigning memory to a null pointer in each case. Aren't we?

Appreciate your response.

[Peter_B]

Switching to malloc will cause memory leaks unless you free the previously allocated memory. Also malloc does not copy over the contents of the previously allocated memory (which is something realloc does do).

So change back to realloc. But be aware that if realloc fails, it returns NULL and leaves the original memory allocated still. This means that you shouldn't put the return value straight back into the old pointer - if it returns NULL you have created a memory leak.

[Lindley]

Why do we need realloc? We are assigning memory to a null pointer in each case. Aren't we?

Appreciate your response.

In the else clause, you are specifically not starting with a NULL pointer.

I will agree it makes no difference in the if (instring==NULL) clause.

[kaftab]

Ok, so back to realloc. But the result is still the same:

// Appends a comma to the given string
char* appendComma(char* inString)
{
	char* outString = NULL;

	if (inString == NULL)
	{	
		printf("\n inside null");
		outString = realloc(NULL, sizeof(","));
		if (outString == NULL)
			return inString;
		strcpy(outString,",");
	}
	else
	{	
		printf("\n inside not null");
		printf("\n instring = %s",inString);
		printf("\n instring len = %d",strlen(inString));

		outString = realloc(inString,strlen(inString) + sizeof(","));
		if (outString == NULL)
			return inString;
		strcat(outString,",");
	}		

	return outString;
}

Output:

1669 Objects - Cycling #: 1
1-Attribute Name: DUMMY
2-Attribute Name: t0GenericID
inside null
3-Attribute Name: t0Variation
inside not null
instring = ,
instring len = 1Segmentation Fault


Thanks for your response.

[Paul McKenzie]

Ok, so back to realloc. But the result is still the same:

Assume inString is an empty string (not NULL, but a string with 0 characters).

		outString = realloc(inString,strlen(inString) + sizeof(","));
		if (outString == NULL)
			return inString;
		strcat(outString,",");

Do you see a problem with this code?

outString = realloc(inString,strlen(inString) + sizeof(","));

So we allocate 0 + 2 bytes = 2 bytes of any random garbage, but still 2 bytes are available we can write to.

strcat(outString,",");

You have random characters starting at outString -- who knows where the terminating NULL is located. So strcat() will start from outString() and search for the first NULL it can find, and who knows where it is -- no one knows. Then when strcat() does find the NULL (which could be thousands of bytes away from outString), it plunks a comma and NULL after it, and who knows where that it is. In your case, it caused a segmentation fault.

The realloc() does not initialize the memory that is returned to you. All you're guaranteed is that you have 2 bytes of available, contiguous data. What's at those two bytes when realloc returns -- you don't know. You have to initialize this memory to '\0' characters before calling strcat().

Regards,

Paul McKenzie

[D_Drmmr]

Assume inString is an empty string (not NULL, but a string with 0 characters).
...
The realloc() does not initialize the memory that is returned to you. All you're guaranteed is that you have 2 bytes of available, contiguous data. What's at those two bytes when realloc returns -- you don't know. You have to initialize this memory to '\0' characters before calling strcat().

If inString is an empty string, then realloc will preserve the contents of that string in case the memory is moved. So outString will start with '\0'.

[kaftab]

Paul,

Thanks for an excellent explanation. So I added the +1 to allow for the null character. You can see from the output that when the function is entered the second time, instring has two characters in it, the "," and the "\0". But I'm still getting the same error!!!

// Appends a comma to the given string
char* appendComma(char* inString)
{
	char* outString = NULL;
	int i = 0;

	if (inString == NULL)
		printf("\ninString is NULL");
	else 
	{
		for (i=0;i < strlen(inString)+1 ;i++ )
			printf("\nchar[%d] = %c - ascii value = %d",i,inString[i],inString[i]);
	}

	if (inString == NULL)
	{	
		printf("\n inside null");
		outString = realloc(NULL, sizeof(",")+1);
		if (outString == NULL)
			return inString;
		strcpy(outString,",");
	}
	else
	{	int i = 0;
		printf("\n inside not null");
		printf("\n instring = %s",inString);


		printf("\n");

		outString = realloc(inString,strlen(inString) + sizeof(",")+1);
	printf("\n---> got here");
		if (outString == NULL)
			return inString;
		strcat(outString,",");
	}		


	return outString;
}

And the output:


1-Attribute Name: DUMMY
2-Attribute Name: t0GenericID
inString is NULL
inside null
3-Attribute Name: t0Variation
char[0] = , - ascii value = 44
char[1] = - ascii value = 0
inside not null
instring = ,
Segmentation Fault

[Peter_B]

Where are these lines in the output coming from:

1-Attribute Name: DUMMY
2-Attribute Name: t0GenericID
3-Attribute Name: t0Variation
?

Can you post a complete, but minimal program which reproduces the error?