1

I am trying to append two chars but for some reason I am getting a segmentation fault.

My code is like;

#include <string.h>
char *one = (char*)("one");
char *two = (char*)("two");

strcat(one, two);

and I seem to be getting a segmentation fault at strcat(one, two), why is that?

daxvena
  • 1,140
  • 3
  • 14
  • 30
  • 3
    What would the memory layout look like before calling `strcat`? What do you expect it to look like afterwards? – Anon. Feb 13 '11 at 21:32
  • Casting should never be done blindly and removing the casts in this case should (because you should have it enabled) [warn you about the deprecated conversion](http://codepad.org/Rb4DzzKW) from string literal to non-const char pointer. That warning will lead you in the right direction to solving this problem, which is given several times in answers below. – Fred Nurk Feb 13 '11 at 21:45
  • 1
    possible duplicate of [Why does simple C code receive segmentation fault?](http://stackoverflow.com/questions/164194/why-does-simple-c-code-receive-segmentation-fault) – Joe Feb 13 '11 at 21:47

9 Answers9

5

http://www.cplusplus.com/reference/clibrary/cstring/strcat/

the first parameter to strcat, must be big enough to hold the resulting string

try:

//assuming a,b are char*
char* sum = new char[strlen(a) +strlen(b)+1];
strcpy(sum,a);
strcat(sum,b);
AK_
  • 7,981
  • 7
  • 46
  • 78
3

There should be enough legal memory to hold the entire string.

char *one = new char[128]; //allocating enough memory!
const char *two = "two"; //"two" is const char*

strcpy(one, "one");
strcat(one, two); //now the variable "one" has enough memory to hold the entire string

By the way, if you prefer using std::string over char* in C++, such thing would be easier to handle:

#include<string>

std::string one = "one";
std::string two = "two";

one = one + two; //Concatenate 

std::cout << one;

Output:

onetwo
Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • 1
    Good explanation. Should advocate for strncat/strncpy and std::string, though. – Michael Aaron Safyan Feb 13 '11 at 21:38
  • Bad example. There's no need for dynamic allocation here: char one[128] = "one"; strcat(one, two);. Why not use one += two in the later std::string example? – Fred Nurk Feb 13 '11 at 21:46
  • @Fred: The OP seems to be newbie; that is why I made it simple and explicit! – Nawaz Feb 13 '11 at 21:49
  • @Nawaz: Are you saying dynamic allocation is simpler? Or that char one[128] isn't explicit? Or that strcat isn't analogous to +=? Either way, I disagree. – Fred Nurk Feb 13 '11 at 21:51
  • 1
    @Fred: Give me downvote if you dislike it. I would not like arguing on these coding-styles and usage. I would not use `char*` for string to begin with. So please spare me! – Nawaz Feb 13 '11 at 21:53
  • This is not an issue of style. I left a comment because I dislike it; I don't downvote. I'm not sure why I should "spare" you for using something badly regardless of why. – Fred Nurk Feb 13 '11 at 21:56
  • @Fred: You're too critical. Not that I don't know what you said. I just thought the OP doesn't need it as of now. – Nawaz Feb 13 '11 at 22:00
  • @Fred: And since you're so critical, then please explain to me why `char one[128]`? why not just `char one[7]`? – Nawaz Feb 13 '11 at 22:02
  • @Nawaz: You're the one that chose 128 without explaining it beyond "enough", not me. As I said, I was addressing the dynamic allocation aspect. – Fred Nurk Feb 13 '11 at 22:03
  • @Fred: But you're the one who criticizing `new`. So while you pick on it, you didn't pick on `128`? Why exactly? Is that because you consider that was good? – Nawaz Feb 13 '11 at 22:05
  • Are you seriously asking why I pointed out a bigger problem but didn't point out a much smaller, nearly non-existent, problem? Are you claiming that misuse of dynamic allocation isn't a problem for "newbies" (as you said you believe the OP to be)? Do you honestly have a problem with me explaining why you have a bad example rather than anonymously downvoting? – Fred Nurk Feb 13 '11 at 22:08
  • @Fred: Exactly. Likewise, I was trying to point out the bigger problem! – Nawaz Feb 13 '11 at 22:10
  • @Fred: Explaining thing is good. But being too too too critical at this stage is not. You could've said what you wanted to say without saying "Bad example". If that is bad, using `char one[128]` is not excellent either! – Nawaz Feb 13 '11 at 22:13
  • 1
    **Teaching someone, especially someone new, to needlessly use dynamic allocation always makes a bad example.** Using a slightly larger than absolutely required char buffer isn't even in the same ballpark. – Fred Nurk Feb 13 '11 at 22:26
  • @Fred: Oh really? That is needlessly? What if I replace that with `char *one = new char[4+ strlen(two)];`. You think what I wrote is final, or what **you** write here on SO cannot be improved? – Nawaz Feb 13 '11 at 22:36
  • Ok seriously, the only reason why I asked this question is because I can't use strings, as in there is almost no point because I would have to convert a char to a string, and then back to a char again. - I have also tried doing that and ended up with another seg fault XP – daxvena Feb 14 '11 at 01:52
2

There are two reasons for this.

  1. If you have a pointer initialized to a string literal, that memory is read-only and modifying it will result in undefined behavior. In this case, if you try to append a string to a string literal, you'll be modifying this sort of memory, which will result in problems.

  2. When using strcat you need to guarantee that space exists for the concatenation of the string at the location you're specifying. In this case, you cannot guarantee that, since a string literal is only guaranteed to have enough space to hold the literal itself.

To fix this, you'll want to explicitly allocate a buffer large enough to hold the concatenation of the two strings, including the null terminator. Here's one approach:

char* buffer = malloc(strlen(one) + strlen(two) + 1);
strcpy(buffer, one);
strcat(buffer, two);

Hope this helps!

templatetypedef
  • 362,284
  • 104
  • 897
  • 1,065
2

The seg fault is because you attempt to write to read only memory. The first action of the strcat is to copy of 't' from the first entry of two into the null at the end of "one". So strictly the seg fault is not due to lack of storage - we never get that far. In fact this code will also likely give you a seg fault:

char* one = "one";
char* two = "";
strcat(one, two);    

All this tries to do is copy a null over a null, but in read-only memory. I suppose a optimiser might happen to stop this on some platforms.

Oddly enough the following (incorrect) code will (probably) not give you a seg fault, and even give the "right" answer:

char one[] = "one";
char two[] = "two";
strcat(one, two);   
printf("%s\n", one);

This successfully writes "onetwo" to stdout on my machine. We get a stack scribble, which we happen to get away with.

On the other hand this does seg fault:

char* one = "one        "; // Plenty of storage, but not writable.
char two[] = "two";
strcat(one,two);    

Hence the solution:

const unsigned enoughSpace = 32;
char one[enoughSpace] = "one";
char two[] = "two";
strcat(one,two);    
printf("%s\n", one);

The issue with this is of course, how large to make enoughSpace in order to store what ever is coming?

Hence the functions strncat, or strcat_s, or more easily std::string.

Moral of the story: in C++, just like C, you really need to know what your memory layout is.

Keith
  • 6,756
  • 19
  • 23
1

You never reserved some space for your strings.

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

int main(void){
    char str[20] = "";
    strcat(str, "one");
    strcat(str, "two");
    printf("%s", str);
}

Would be one correct way to do this. The other (and way better) is to use the std::string class.

#include <string>
#include <cstdio>

int main(void){
    std::string str;
    str += "one";
    str += "two";
    std::printf("%s", str.c_str());
}
Xeo
  • 129,499
  • 52
  • 291
  • 397
  • Why the using directive in the first example? – Michael Aaron Safyan Feb 13 '11 at 21:38
  • @Michael: I include the C++ versions of the C-header (c at front and no .h at end). Call it a habbit. ;) – Xeo Feb 13 '11 at 21:46
  • Why not put the using directive at function scope? – Fred Nurk Feb 13 '11 at 21:49
  • @Fred: Good question, I never really thought about that. I only use the using directive in .cpp files and thinking back, I always put them at global scope.. Any reason I should change that habbit? – Xeo Feb 13 '11 at 21:55
  • 2
    @Xeo: Using directives often lead to problems that habitually localizing their effect makes easier to solve (for example, there's a problem with std::endl and Qt). In this case, like Bo Persson, I would simply type "std::" twice. – Fred Nurk Feb 13 '11 at 22:01
1

There are several problems here. Firstly, though you have casted the strings to mutable versions, they really are string literals and hence should not be written. Secondly, you are using strcat which will write into the string buffer, completely ignoring the length of the string buffer (it's better to use strncat which requires you to specify the length of the buffer). Lastly, since this is C++, it would be way better to use:

#include <string>

// ...

string one = "one";
string two = "two";
one.append(two); 
Michael Aaron Safyan
  • 93,612
  • 16
  • 138
  • 200
0

strcat needs a "writeable" buffer as the target. In your example, it is a pointer to a string constant (or literal), which you cannot write to, thus it results in an exception. The target buffer can be a buffer on the stack or one dynamically allocated (e.g., with malloc).

Mark Wilkins
  • 40,729
  • 5
  • 57
  • 110
0

Your destination string should be large enough to hold both the destination and the source string. So an example would be

char one[10] = "one";
char two[4] = "two";
strcat(one,two);

Skyler Saleh
  • 3,961
  • 1
  • 22
  • 35
0

This is not the problem of "not enough space".

char *a = "str";

Look at the code above, the pointer a is point to "static memory". The string "str" is stored in the static place in the PCB, which means it can't be overwritten.

So, the codes below will be better:

#include <string>
using std::string;

string a = "stra";
string b = "strb";

a += b;
Ace
  • 345
  • 2
  • 3
  • 11