Solved

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

Posted on 2003-11-14
13
1,592 Views
Last Modified: 2013-12-14
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?
0
Comment
Question by:rayskelton
  • 4
  • 3
  • 2
  • +3
13 Comments
 
LVL 16

Assisted Solution

by:_nn_
_nn_ earned 250 total points
ID: 9749210
>> 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)
0
 

Author Comment

by:rayskelton
ID: 9749424
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.

0
 
LVL 16

Expert Comment

by:_nn_
ID: 9749688
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.
0
Netscaler Common Configuration How To guides

If you use NetScaler you will want to see these guides. The NetScaler How To Guides show administrators how to get NetScaler up and configured by providing instructions for common scenarios and some not so common ones.

 
LVL 45

Assisted Solution

by:Kdo
Kdo earned 200 total points
ID: 9749774

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
0
 
LVL 11

Expert Comment

by:dimitry
ID: 9749858
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...
0
 
LVL 45

Expert Comment

by:Kdo
ID: 9749901

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
0
 
LVL 3

Expert Comment

by:guynumber5764
ID: 9752357
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).
0
 
LVL 22

Accepted Solution

by:
grg99 earned 50 total points
ID: 9754049

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.

0
 

Author Comment

by:rayskelton
ID: 9763484
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.
0
 

Author Comment

by:rayskelton
ID: 9766863
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);
}

0
 
LVL 22

Expert Comment

by:grg99
ID: 9767125
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.



0
 
LVL 45

Expert Comment

by:Kdo
ID: 9767166

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);
}
0
 

Author Comment

by:rayskelton
ID: 9767177
I agree the Linked list code needs some serious scrutiny and is high on my list.

 Thanks to all.
0

Featured Post

Simplifying Server Workload Migrations

This use case outlines the migration challenges that organizations face and how the Acronis AnyData Engine supports physical-to-physical (P2P), physical-to-virtual (P2V), virtual to physical (V2P), and cross-virtual (V2V) migration scenarios to address these challenges.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Programmer's Notepad is, one of the best free text editing tools available, simply because the developers appear to have second-guessed every weird problem or issue a programmer is likely to run into. One of these problems is selecting and deleti…
Update (December 2011): Since this article was published, the things have changed for good for Android native developers. The Sequoyah Project (http://www.eclipse.org/sequoyah/) automates most of the tasks discussed in this article. You can even fin…
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use nested-loops in the C programming language.
The goal of this video is to provide viewers with basic examples to understand opening and reading files in the C programming language.

773 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question