What is wrong with this program?

Question

I am trying to write a string concatenation code. I wonder what is wrong in it. Can you guyz help me out. Here is my code.

#include <stdlib.h>

void strcat1(char *s, char *t)
{
    while(*s !='\0')
        s++;

    while((*s++=*t++)!= '\0')
    {

    }

}

int main()
{
char s[]= "hello";
char t[]= "world";
strcat1(s,t);
printf("%s", s);

return 0;
}

I am getting this output on codepad.org: Disallowed system call: SYS_socketcal Here is the link: http://codepad.org/Arz6U7YA

EDIT: Will the change char *s = "Hello" and char *t= "World" in the main function make any differenvce?

Deleted my comment; this program *does* print "helloworld" when I compile it with GCC, but could segfault instead. — Wooble, Apr 06 '11 at 22:10
Re EDIT: The change `char *s = "Hello";` will make things worse. Now you have both an array out-of-bounds problem and a modifying constant data problem. — aschepler, Apr 06 '11 at 22:11
oh ya correct. Char *s would save data as literal in data segment. But what the current case? — prap19, Apr 06 '11 at 22:14

pmg · Accepted Answer · 2011-04-06T22:12:43.517

8

s has space for 6 chars (namely 'h', 'e', 'l', 'l', 'o', and '\0').

You are trying to write there 5 more characters than it can hold.

Don't do that!

Try increasing the size of s before

int main()
{
char s[11] = "hello";
/* ... */

Edit after the edit of the OP

Changing s in main to

char *s = "hello";

changes s from an array with little space to a pointer to a string literal.

String literals are not modifiable, so you can't expect your code to work with the change.

edited Apr 06 '11 at 22:12

answered Apr 06 '11 at 22:06

pmg

106,608
13
126
198

how does library strcat work in that case? Does it reallocate memory to the source string and then copy target string? – prap19 Apr 06 '11 at 22:13
No. The pointer can point to a string literal (`s = "hello";` is ok), but the string literal itself cannot be changed (`*s = 'w';` is wrong). Note one expression has the `'*'` and the first doesn't dereference the pointer. – pmg Apr 06 '11 at 22:15
so could you give me an example of case when I can write *s = 'w' where *s is previously defined? – prap19 Apr 06 '11 at 22:18
thats a pretty good example. So what is best solution for my problem? – prap19 Apr 06 '11 at 22:26
You have a few options: the best varies according to your specific needs. Two options that come to mind now are to copy the standard `strcat` as you did: *then you must ensure the destination has enough space for the resulting string*; or can allocate space for the resulting string and return that (as in `char *strcat2(const char *part1, const char *part2);`) – pmg Apr 06 '11 at 22:35
But what if I don't want to return a string. The changes should reflect in the source string argument passed? – prap19 Apr 06 '11 at 22:41
1

You should do like the Standard: in the documentation for your function, specify that accessing beyond the end of objects is undefined behaviour :) – pmg Apr 06 '11 at 22:49

score 3 · Answer 2 · answered Apr 06 '11 at 22:06

3

Buffer overflow. You cannot append to the end of array s because it was allocated with only 6 characters (5 printable and one \0).

answered Apr 06 '11 at 22:06

aschepler

70,891
9
107
161

score 3 · Answer 3 · answered Apr 06 '11 at 22:06

3

Well, the big problem is that s isn't large enough to hold the result; it's sized to hold 6 characters (5 letters plus the 0 terminator), so as soon as you start trying to append the contents of t, you overrun the buffer.

answered Apr 06 '11 at 22:06

John Bode

119,563
19
122
198

score -3 · Answer 4 · answered Apr 06 '11 at 22:06

-3

You are trying to append to a constant string. That memory segment is protected. Create a new buffer, concat in there, and then return that.

answered Apr 06 '11 at 22:06

drudru

4,933
1
19
19

4

Incorrect. `s` is an array of char; not a pointer to read-only memory. – pmg Apr 06 '11 at 22:06
1

No, he's trying to append to `s`, not the string literal itself. The problem is that the target buffer is too small for the result. – John Bode Apr 06 '11 at 22:07
wow - C has changed a bit in the 20 years since I've used it. Is that a C99 thing? – drudru Apr 06 '11 at 22:13
1

It was like we describe pre-ANSI C89. My version of K&R (version 2) uses the term "string constant" but the behaviour is the same (see page 104). – pmg Apr 06 '11 at 22:20
@drudru: Nothing's changed, at least not since C89; again, he's not trying to append to the literal, he's trying to append to the auto array that was *initialized* from the literal. – John Bode Apr 06 '11 at 22:34
If you don't want to fix this post, now would be an ideal time to get your very own [peer pressure badge](http://stackoverflow.com/badges/38/peer-pressure). – dmckee --- ex-moderator kitten Apr 07 '11 at 01:44

What is wrong with this program?

4 Answers4