Printable View
Hi Gurus,
Trying to append a comma to a string. Getting "Segmentation Error" on Solaris when the function is entered the second time.
Your help is alwasy appreciated.Code:
// 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,",");
}
}
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.
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:
but that didn't change the outcome.Code:instring = realloc(instring, strlen(instring) + strlen(",")+1);
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.)
You have this in your code:Quote:
Originally Posted by kaftab
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.
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:
Code:// 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;
}
I don't see any reason to switch from realloc to malloc here. You still need realloc's functionality.
Why do we need realloc? We are assigning memory to a null pointer in each case. Aren't we?
Appreciate your response.
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.
In the else clause, you are specifically not starting with a NULL pointer.Quote:
Originally Posted by kaftab
I will agree it makes no difference in the if (instring==NULL) clause.
Ok, so back to realloc. But the result is still the same:
Output:Code:// 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;
}
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.
Assume inString is an empty string (not NULL, but a string with 0 characters).Quote:
Originally Posted by kaftab
Do you see a problem with this code?Code:outString = realloc(inString,strlen(inString) + sizeof(","));
if (outString == NULL)
return inString;
strcat(outString,",");
So we allocate 0 + 2 bytes = 2 bytes of any random garbage, but still 2 bytes are available we can write to.Code:outString = realloc(inString,strlen(inString) + sizeof(","));
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.Code:strcat(outString,",");
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
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'.Quote:
Originally Posted by Paul McKenzie
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!!!
And the output:Code:// 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;
}
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
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?
My feeling is that you're trying to do something else with strings that are faulty. But none of us will know until we see a main() program that duplicates your error.Quote:
Originally Posted by kaftab
However, the one flaw with your appendComma function is that you're either returning outString, which was dynamically allocated, or inString, and who knows where that may have come from. So if you have a non-trivial program, how would you know if you need to free() the return value of appendComma?
This code will fail if the realloc() returned NULL inside of appendComma, since you're freeing a string literal (the inString would be returned, which would be "abc123"). I mention non-trivial program, since you would really have to write some overly complex code to keep track of what was returned, whether it is the same address as the item passed in, etc. in several places in the program. You're bound to make a mistake somewhere in this scenario, so the appendComma() function may be a learning exercise, but for all practical purposes, could not be used in a real-world, critical, C application.Code:char *myNewString = appendComma("abc123");
free( myNewString );
The issue with 'C' strings is this -- string concatenation, getting a substring from a string, etc. need to be well thought-out, even though it seems simple to do on the surface.
Some C programmers have even resorted to creating a mini "API" just for handling of strings correctly in 'C', just to ensure that their app will not crash, similar to creating the C++ std::string class, but using 'C' instead. Possibly something like this:
something along those lines. Now the creation of the string is hidden, but controlled. The concatenation is now controlled, as it will accept only dynamic strings created with CreateDynamicString. Then the FreeDynamicString() can do a check to see if the string was actually allocated, and if so, free() can be called. An even more elaborate scenario would be to create a struct that holds information about the pointer, how many characters, etc, instead of a naked char pointer being used (similar to the std::string example I mentioned).Code:char *ptr = CreateDynamicString(); /* cretes a dynamic string */
ConcatDynamicString(ptr, "abc123");
FreeDynamicString( ptr );
Regards,
Paul McKenzie
Yes, you're right. I forgot about realloc() preserving the original contents if the argument isn't NULL.Quote:
Originally Posted by D_Drmmr
Regards,
Paul McKenzie
I got your point Peter. I removed all the other stuff and I cannot reproduce the problem now. This prompted me to refactor my code and start fresh.Quote:
Originally Posted by Peter_B
Thanks.