strcat is causing seg fault (c program)

mfrick · 03-14-2006, 08:28 PM

Hi peoples,
I have a bit of code that compiles perfectly but when I run it I get a segmentation fault (core dumped) which is not fun. I have broken up my code (I had some nested functions etc) and found that the error is being generated at the below line of code.

Code:

temp = strcat("evusrseq=\"",temp1);

*not the most descriptive variable names but only used to break up the code to find the line that breaks. temp & temp1 are both strings.

** this code is being compiled by gcc v 2.95.3 on SunOS 5.9 if that makes any difference.

Basically this bit of c code looks fine to me as I have used strcat in other places with no problem so if anyone can see any reason that this would seg fault I would appreciate knowing why also

(I am hoping it is something obvious that I am just missing that when found makes me look like an idiot)

jschiwal · 03-14-2006, 09:20 PM

The destination should be a pointer to a character string instead of a pointer to a constant. You also need to make sure that you don't end up with a string larger then the memory you allocated. That could cause a runtime error which is harder to find.

paulsm4 · 03-14-2006, 10:00 PM

jschiwal is absolutely correct: you should *never* try to modify a string literal:

Code:

  // WRONG
  char *temp1 = "something";
  strcat("evusrseq=\"",temp1);

  // BETTER
  char *temp1 = "evusrseq=\"                 ";
  char *temp2 = "something";
  char temp1[10] = 0;
  strcat(temp1,temp2);

  // BEST
  char temp1[MAX_STRING];
  strncpy (temp1, "evusrseq=\"", MAX_STRING);
  strncat (temp1, "something", MAX_STRING);

mfrick · 03-14-2006, 10:27 PM

Thanks guys you were 100% correct

I think I was thrown by the fact that I wasn't recieving any errors on a windows implementation that was using similar code.

It has been quite a few years so I guess my c rust has built up more than I had hoped.

Once again thanks for your help.

Gamecock · 08-12-2014, 09:20 AM

@paulsm4 your implementation for strncat would overflow. The n is number of char to copy, not number of char in destination total. See http://www.cplusplus.com/reference/cstring/strncat/

The safest representation is:
// BEST
char temp1[MAX_STRING];
strncpy (temp1, "evusrseq=\"", MAX_STRING);
strncat (temp1, "something", sizeof(temp1)-strlen(temp1));

NevemTeve · 08-12-2014, 10:39 AM

That's what snprintf is good for...

mina86 · 08-13-2014, 05:22 PM

Quote:

Originally Posted by paulsm4

Code:

// BEST
  char temp1[MAX_STRING];
  strncpy (temp1, "evusrseq=\"", MAX_STRING);
  strncat (temp1, "something", MAX_STRING);

No, it is not! It will fail if MAX_STRING == strlen("evusrseq=\""). Also, strncat's third argument is not the length of the buffer, but maximum amount of characters to read from the source. I.e. strncat is totally broken.

Choose one of:

Code:

char temp[…];
temp[0] = 0;
strncat(temp, "foo", sizeof temp - 1);
strncat(temp, "bar", sizeof temp - 1 - len(temp));

Code:

char temp[…];
strncpy(temp, "foo", sizeof temp);
temp[sizeof temp - 1] = 0;
strncat(temp, "bar", sizeof temp - 1 - len(temp));

Code:

char temp[…];
snprintf(temp, sizeof temp, "%s%s", "foo", "bar");

Of course, if some of the strings are actual literals, snprintf with proper format is most readable, eg.:

Code:

char temp[…];
snprintf(temp, sizeof temp, "foo%s", "bar");

Quote:

Originally Posted by Gamecock

@paulsm4 your implementation for strncat would overflow. The n is number of char to copy, not number of char in destination total. See http://www.cplusplus.com/reference/cstring/strncat/

The safest representation is:
// BEST
char temp1[MAX_STRING];
strncpy (temp1, "evusrseq=\"", MAX_STRING);
strncat (temp1, "something", sizeof(temp1)-strlen(temp1));

Still not safe. You need to subtract 1 for the NUL byte.

zer0python · 08-15-2014, 07:21 PM

Just to note, doing

Quote:

char temp1[MAX_STRING];

is using memory on the local stack. Which can cause issues if the code is being executed in a thread.
my two cents:

Code:

char *temp1 = malloc(MAX_STRING+1);
if(temp1 == NULL) {
  /* bail */
}

sprintf(temp1, "evusrseq=\"%s\", temp2);
/* where strlen(temp2) will never be > MAX_STRING-strlen("evusrseq=\"\""); */
/* ... use temp1 ... */
free(temp1);
temp1 = NULL; /* pedantic */

Edit: This *is* lazy coding, I'm aware using malloc(strlen(temp2) + ... + 1) would be ideal for memory constraints.

NevemTeve · 08-16-2014, 07:09 AM

What about asnprintf?

mina86 · 08-20-2014, 07:34 AM

Quote:

Originally Posted by NevemTeve

What about asnprintf?

One must remember it's non-standard (even though easy to implement with snprintf if efficiency is not a concern).