Sprintf() causing core dump sporadically on old existing code.

Sun Solaris 2.8 running in 32 bit mode with 6.2 compiler.
I've been running into a problem sporadically in the use of sprintf:

sprintf(char_string, " ... %s", function());

where function() returns a character string

This has been causing core dumps in some places (function return is probably in limbo), often in old code that has been in production for years.  It has been popping up when I make changes, usually completely unrelated, other places in the code.  So far it has always been caught in development, but may become a production issue.

***WHY***
To fix this copy the function return to a character array and pass that in to sprintf.

This has been a rare bug so far, but you may more of a problem and I need to come up with the cause. Any ideas?
rayskeltonAsked:
Who is Participating?

[Product update] Infrastructure Analysis Tool is now available with Business Accounts.Learn More

x
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

_nn_Commented:
>> where function() returns a character string

Depends on where that returned string was allocated. If it's a simple variable local to function() (a char[]) then it's just invalid. If it's a static buffer, then you could investigate multi-threading issues. More informations about that function wouldn't hurt...

Finally, sprintf() is dangerous per se, how do you guaranty that char_string is large enough ? Better use snprintf (if supported, I don't know Solaris compilers)
rayskeltonAuthor Commented:
The char_string in sprintf(char_string, " ... %s", function()); is always allocated and has been running for years, but just recently became an issue. If you allocate a seperate buffer to hold the result of sprintf(), then copy this buffer into the destination string, everyone is happy.

_nn_Commented:
Ok, if you don't mind, let us put the issue about sprintf itself by the side. I disagree with you about that C standard function and specially about things that have been "running for years", but we'll come back to it after we've checked what looks more important to me.

Generally speaking, the amount of informations and source code you're revealing is pretty tiny, specially when trying to resolve a "sporadic crash". When I said "More informations about that function wouldn't hurt...", I was talking about the called some_function in

sprintf(char_string, " ... %s", some_function());

Can you please give some more details about some_function() ? What exactly is returned and how ? Again, is that application multi-threaded or not ? When you say :

>> To fix this copy the function return to a character array and pass that in to sprintf.

... it suggests that something wrong is going on within that function. If possible, paste the source code here.

Thanks.
Become a Microsoft Certified Solutions Expert

This course teaches how to install and configure Windows Server 2012 R2.  It is the first step on your path to becoming a Microsoft Certified Solutions Expert (MCSE).

Kent OlsenDBACommented:

Hi rayskelton,

You can definitively prove (or disprove) if the sprintf() is the cause of the failure or if something else is clobberring your program and it only shows up at this point.

Redefine char_string as dynamic and make a new variable for holding the value returned by function().  Also create a static variable with your format string.

char *char_string = NULL;
char *function_value;
char *FormatString = ".... %s";

Then change the sprintf() to look like this:

function_value = function ();
char_string = realloc (char_string, strlen(FormatString) + strlen (function_value));
sprintf (char_string, FormatString, function_value);


If the code works, then the string generated by sprintf() was longer than was allocated for char_string.  If the code still fails, the problem lies elsewhere in your program.

I assume that your format string is more complicated that what you indicated so make sure that you realloc() a buffer large enough for the string.  The length generated in my example is sufficiently large for this example because the "%s" substring is counted for determining the length, but will be dropped when the substitution is made -- hence the buffer is long enough for the data and the terminating zero.


Good Luck!
Kent
dimitryCommented:
It may be important to check _nn_ suggestion. Can you please show use function() code ?
Not the whole code but the part where is return value is allocated...
Kent OlsenDBACommented:

Hi Ray,

One more thing to check...  Does function() ALWAYS return a non-null value?  There is no guarantee when dereferencing NULL so you could wind up with an error that results in a core dump.  From my previous example, add 1 line so the call to function() is now these two lines:

function_value = function ();
function_value = function_value ? function_value : "<function returned null>";


Again, Good Luck,
Kent
guynumber5764Commented:
The two main things that'll cause sprintf(sdst, "%s", ssrc) to crap out are 1) ssrc==NULL and 2) strlen(ssrc) > sizeof(sdst) (usually because ssrc was not terminated).
grg99Commented:

It's always risky to use sprintf() with a %s format string.... How do you know the string is going to fit in the destination char array?.   Much better to use snprintf().  Also it wouldnt hurt to add some checks that the function() always returns a proper char * that isnt NULL or garbage.  And the function() better not return a pointer to a char array local to function(), as that no longer exists by the time sprintf() gets called.

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
rayskeltonAuthor Commented:
Therse are many good suggestions. I will try some of these today and try to get some code samples posted. Thanks to all for suggestions.
rayskeltonAuthor Commented:
The offending code is listed below. The snprintf() resolves all related core dumps, so I am happy with that solution. The List class is around 12 years old and used in thousands of places through the company, so I want to avoid this if possible. This is the method returned in the sprintf(). This just began around siz months ago and is around the same time we converted this code to 64 bit. We have had several bad behaving occurrences of old code, since the upgrade. ll->data is a node within a linked list.

int kb215_add_reason_codes(List & msg_list, NcfResult & ncf)
{
  List *ll = (List *) &ncf.pls_mssg();
  for(ll->rewind(); ll->data() ; (*ll)++ )
  {
    char temp[80];
    sprintf(temp, (char *)"  %s", (char *)ll->data());
    msg_list.append(ll_strdup(temp));
  }

  return(ERR_OK);
}

grg99Commented:
The sprintf is being used here as a slow and disaster-prone way to put a space in front of a string.

This is a bit faster and safer:

char temp[ 80 ];

strncpy( temp, "  ", sizeof( temp ) );
strncat( temp, ll->data(), sizeof( temp ) );

-------------

You might also try adding a few if() statements to catch the array overrun condition and log it somewhere.
This little problem may be a symptom of a larger problem somewhere else.  Maybe somehow bad data is getting
into your lists.  It would be nice to track down the source of the problem instead of just patching this hole.



Kent OlsenDBACommented:

Hi Ray,

sprintf() may have been the quick and easy way to put a space in front of a string (message), but it sure did cause a headache, huh?  If you malloc() a buffer that's large enough for the new string you should be just fine...

Congratulations on isolating the problem!
Kent


int kb215_add_reason_codes(List & msg_list, NcfResult & ncf)
{
  char *temp = NULL;
  List *ll = (List *) &ncf.pls_mssg();
  for(ll->rewind(); ll->data() ; (*ll)++ )
  {
    temp = (char *) realloc (temp, strlen (ll->data)+1+1);  /*  Add one for the leading space, and one more for the zero terminator  */
    strcpy (temp, " ");
    strcat (temp, (char *)ll->data());
    msg_list.append(ll_strdup(temp));
  }
  free (temp);
  return(ERR_OK);
}
rayskeltonAuthor Commented:
I agree the Linked list code needs some serious scrutiny and is high on my list.

 Thanks to all.
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Editors IDEs

From novice to tech pro — start learning today.