Implementing strcat() using pointers

Question

I am reading K&R and trying to do the excercise involving writing a version of strcat (attach one string to the end of the other) using pointers. This is what I have:

#include<stdio.h>
void mystrcat(char *s,char *t)
{
    while(*s++);
    *s--;
    while(*s++ = *t++);
}

int main()
{
    int size = 1024;
    char *s1, *s2;
    s1 = malloc(size);
    s2 = malloc(size);
    s1 = "Hello ";
    s2 = "World";
    mystrcat(s1,s2);
    printf("%s",s1);
    return 0;
}

The program runs but crashes straight away, not very experienced with pointers so can't work out the error.

Does K&R use `malloc` there already? Which exercise is it? Should be pointer chapter... — Ely, Dec 15 '15 at 18:30
Always check (!=NULL) the returned value from `malloc()` to assure the operation was successful. Then pass those pointers to `free()` before exiting the program. — user3629249, Dec 16 '15 at 19:20
regarding lines like: `s1 = "Hello ";`. This overlays the pointer returned from malloc with the ptr to "Hello ", thereby forever losing the original pointer, resulting in a memory leak. Suggest: `char * ptr = "Hello "; for( int i=0; *ptr[i]; i++ ) { s1[i] = ptr[i]; } s1[i] = '\0'; There is no need to malloc the s2, as that array will not be changing. Instead say: `s2 = "World"; — user3629249, Dec 16 '15 at 19:31
I would replace these two lines: `while(*s++); *s--;` with: `for( ; *s; s++ );` then no need for corrections to the 's' pointer — user3629249, Dec 16 '15 at 19:35

Baltasarq · Accepted Answer · 2015-12-15T18:46:41.627

7

The problem is here:

s1 = malloc(size);
s2 = malloc(size);
s1 = "Hello ";
s2 = "World";

You have allocated space for s1 and s2, but instead of using it, you are making them point to string literals. These string literals are stored in a non-writable place of the memory (i.e., you should not mess with it).

In short, you have to change your code to:

s1 = malloc(size);
s2 = malloc(size);
strcpy( s1, "Hello " );
strcpy( s2, "World" );

Now you are using the allocated space. Hope this helps.

EDITED: you also have a problem here:

void mystrcat(char *s,char *t)
{
    while(*s++);
    *s--;
    while(*s++ = *t++);
}

The second line should be s--, instead of *s--. *s-- would decrease the pointer s and access that new position. You don't actually need to dereference the pointer just to decrease it. You just want the pointer to point to a position before it is pointing to now. That's simply s--.

edited Dec 15 '15 at 18:46

answered Dec 15 '15 at 18:27

Baltasarq

12,014
3
38
57

The first (and most prominently presented) is not the reason for the crash, but for a memory leak (only). – too honest for this site Dec 15 '15 at 18:36
`*s--` does not decrement a `char`, it decrements the pointer. But it does dereference a pointer likely to be outside an array bound (and then discard that value). – aschepler Dec 15 '15 at 18:38
1

In a PC, yes, it is probable that it won't crash. But that does not mean it is correct. That's the famous undefined behaviour. It could potentially make the program crash in your PC, even if it does not do it. Now imagine a program compiled for a Gameboy Advance. The literals would be stored with the code in the cartridge (read-only memory), and it would certainly crash, 100%. – Baltasarq Dec 15 '15 at 18:39

score 2 · Answer 2 · answered Dec 15 '15 at 18:29

2

You made another mistake in *s--. Pointer s is out of the buffer, and you shouldn't dereference it.

void mystrcat(char *s,char *t)
{
    while(*s++);
    s--;
    while(*s++ = *t++);
}

answered Dec 15 '15 at 18:29

ainoss

195
1
1
9

score 1 · Answer 3 · answered Dec 15 '15 at 18:27

You allocate memory for the strings, but then overrides it and lose the reference to this memory:

// allocate memory
s1 = malloc(size);
s2 = malloc(size);
// assign another address, e.g. lose the pointer
s1 = "Hello ";
s2 = "World";

You should copy the string to the allocate memory:

// allocate memory
s1 = malloc(size);
s2 = malloc(size);
// copy the string into it
strcpy(s1, "Hello ");
strcpy(s2, "World");

Also, it is a good practice to always free the memory you allocate after you are done with it.

score 1 · Answer 4 · answered Dec 15 '15 at 18:28

s1 = "Hello ";

Now s1 is pointing at a read-only array of 7 characters storing the literal "Hello ". You've lost the pointer to the allocated 1024 characters that s1 was pointing at before. So your attempt to append to s1 is illegal.

Try maybe strcpy(s1, "Hello "); instead.

score 1 · Answer 5 · answered Dec 15 '15 at 18:31

You can't copy a string by simple assignment.

If you want to copy "Hello " into a memory allocated with malloc, you need to copy the string into that block of memory:

s1 = malloc(size);
s2 = malloc(size);
strcpy(s1, "Hello ");
strcpy(s2, "World");

Direct assignment doesn't work with arrays.

"Hello " is essentially a char array.

Also, like someone said in the comments, "Hello " is allocated in a read only section of the code (Since it's allocated during compile time), so when you used your assignment, you actually redirect s1 to a read-only location, and when you try to write in a read only location, you're going to have a bad time.

Vlad from Moscow · Answer 6 · 2015-12-15T18:45:19.903

First of all the function would look better if to define it the following way

char * mystrcat( char *s1, const char *s2 )
{
    char *p = s1;

    while( *s1 ) ++s1;
    while( *s1++ = *s2++ );

    return p;
}

The standard C function strcat returns pointer to the destination string.

As for the main then there are memory leaks because at first s1 and s2 are set to the addreses of the allocated memory and then they are are reassigned by the addresses of first characters of the string literals.

And moreover string literals in C and C++ are immutable. Any attempt to modify a string literal results in undefined behaviour of the program.

You could write instead

int size = 1024;
char *s;

s = malloc(size);
s[0] = '\0';

mystrcat( s, "Hello " );
mystrcat( s, "World" );

printf( "\"%s\"\n", s );

Or even you could write in one line

printf( "\"%s\"\n", mystrcat( mystrcat( s, "Hello " ), "World" ) );

Implementing strcat() using pointers

6 Answers6