6

why does this code crash? is using strcat illegal on character pointers? 

#include <stdio.h>
#include <string.h>

int main()
{
   char *s1 = "Hello, ";
   char *s2 = "world!";
   char *s3 = strcat(s1, s2);
   printf("%s",s3);
   return 0;
}

please give a proper way with referring to both array and pointers.

enzo
  • 9,861
  • 3
  • 15
  • 38
Ashish Yadav
  • 1,667
  • 6
  • 20
  • 26
  • 2
    you are using strcat wrong. It appends the second string to the end of the first string. The string it returns is just a convenience. You cannot alter a constant string (your s1), that is why it crashes. s1 points to read-only memory. – Jason Coco Mar 13 '10 at 05:08
  • 14
    If everything was perfectly fine, the code wouldn't crash. – James McNellis Mar 13 '10 at 05:08
  • 1
    You might want to edit your question, Ashish. You are probably getting downvotes because you say "even tho everything is perfectly fine". It's a very valid question, though. – Jason Coco Mar 13 '10 at 05:17
  • 2
    thanks jason i don't bother about downvotes as far as i am getting knowledge from you fine people.i will follow your advice from now on. thanks again you are really kind. – Ashish Yadav Mar 13 '10 at 05:26
  • 1
    @ashish: try to format your questions correctly to get the proper answers. In the edit box while asking questions there are many formatting buttons available. You have been member long enough. So even now, if you dont bother to format your questions, you will get downvotes. – Naveen Mar 13 '10 at 05:26
  • sorry guys i apologize but i have never been good at writing readable codes but i would love to learn them.well thanks everyone . – Ashish Yadav Mar 13 '10 at 05:32
  • 2
    I think he was referring to putting the code into a code block. When adding code to a question or answer, you can highlight all of the code text and press the "101 010" button above the text box to format it nicely. – Josh Townzen Mar 13 '10 at 05:44
  • 2
    You should declare string literals as const char*, then the compiler would catch problems like this for you. – Dan Olson Mar 13 '10 at 10:23

3 Answers3

12

The problem is that s1 points to a string literal and you are trying to modify it by appending s2 to it. You are not allowed to modify string literals. You need to create a character array and copy both strings into it, like so:

char *s1 = "Hello, ";
char *s2 = "world!";

char s3[100] = ""; /* note that it must be large enough! */
strcat(s3, s1);
strcat(s3, s2);
printf("%s", s3);

"Large enough" means at least strlen(s1) + strlen(s2) + 1. The + 1 is to account for the null terminator.

That having been said, you should seriously consider using strncat (or the arguably better but nonstandard strlcat, if it is available), which are bounds-checked, and thus are far superior to strcat.

James McNellis
  • 348,265
  • 75
  • 913
  • 977
  • Variable length arrays (c99) or anything less than 100! :P –  Mar 13 '10 at 06:32
  • The reason that `strcat` returns its first argument is a convenience for situations just like this one - it means you can do the concatenation in one line: `strcat(strcat(s3, s1), s2);` – caf Mar 13 '10 at 07:49
  • I'd actually prefer snprintf to strncat in almost every case like this. It's a minor performance hit, but more likely to be used correctly since strncat's correct usage is inconsistent with the rest of the library. (The 'n' means something different than people seem to think.) – Dan Olson Mar 13 '10 at 08:17
  • @rogue: I picked something obviously large enough, then explained what "large enough" really means. You are right that in this case, 100 chars are not required. @caf: That's good to know; I wasn't sure off the top of my head what `strcat` returned (that's what I get for having lived in C++land for too long ;-)). @Dan: Agreed that `strncat` is confusing and difficult (hence the mention of `strlcat`). Actually, `snprintf` might be faster for long strings or a large number of strings since it wouldn't have to walk the destination string to find the terminator for each string being concatenated. – James McNellis Mar 13 '10 at 14:28
  • Pointers to string literals should usually be declared const, and that would have turned the run-time crash into a compile-time warning, making it easier to find the problem. – Adrian McCarthy Mar 01 '13 at 16:56
1

The correct way in this case would be to allocate enough space in the destination string(s1) to store 6 extra characters(s2) as well as the null terminator for the string. 

char s1[14] = "Hello, ";
char *s2 = "world!";
char *s3 = strcat(s1, s2);
printf("%s",s3);
penguin4hire
  • 288
  • 1
  • 2
  • 14
0

Here is a quote from the strcat() manual : "The strcat() function appends the src string to the dest string, overwriting the null byte ('\0') at the end of dest, and then adds a terminating null byte. The strings may not overlap, and the dest string must have enough space for the result."

The problem here is that s1 and s2 point to static strings which are "read only", so if you try to do a strcat operation, with such a string in the dest parameters, you will get an error.

The best way to create your hello world string here is to malloc it so it will be able to contain both s1 and s2. Also, don't forget to add a '\n' at the end of your printf format string otherwise you might be surprised.

Here is the code I would write if I were you :


int main()
{
  char* s1 = "Hello ";
  char* s2 = "World !";
  char *s3 = malloc((strlen(s1) + strlen(s2) + 1) * sizeof(char));
/* +1 is for the null terminating character
and sizeof(*s3) is the actual size of a char. */

  if (s3)
  {
    strcat(s3, s1);
    strcat(s3, s2);
    printf("%s\n", s3);
    free(s3); // always free what you alloc when you don't need it anymore.
  }
  return 0;
}
Opera
  • 983
  • 1
  • 6
  • 17