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
– Ely, Commented Dec 15, 2015 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
– user3629249, Commented Dec 16, 2015 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
– user3629249, Commented Dec 16, 2015 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
– user3629249, Commented Dec 16, 2015 at 19:35

Baltasarq · Accepted Answer · 2015-12-15 18:46:41Z

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, 2015 at 18:46

answered Dec 15, 2015 at 18:27

Baltasarq

12.4k3 gold badges41 silver badges59 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

too honest for this site Over a year ago

The first (and most prominently presented) is not the reason for the crash, but for a memory leak (only).

aschepler Over a year ago

*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).

Baltasarq Over a year ago

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%.

ainoss · Accepted Answer · 2015-12-15 18:29:12Z

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, 2015 at 18:29

ainoss

3251 gold badge2 silver badges9 bronze badges

Comments

MByD · Accepted Answer · 2015-12-15 18:27:50Z

1

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.

answered Dec 15, 2015 at 18:27

MByD

138k30 gold badges269 silver badges278 bronze badges

Comments

aschepler · Accepted Answer · 2015-12-15 18:28:41Z

1

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.

answered Dec 15, 2015 at 18:28

aschepler

72.5k11 gold badges114 silver badges174 bronze badges

Comments

NadavL · Accepted Answer · 2015-12-15 18:31:18Z

1

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.

answered Dec 15, 2015 at 18:31

NadavL

4002 silver badges13 bronze badges

Comments

Vlad from Moscow · Accepted Answer · 2015-12-15 18:45:19Z

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" ) );

Collectives™ on Stack Overflow

Implementing strcat() using pointers

6 Answers 6

3 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

3 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related