CODEWITHSUNDEEP
c
AGeek
AGeek

Reputation: 5225

concatenation program in C

I have written a program for concatenation two strings, and it is throwing segmentation fault during run time at line s1[i+j] = s2[j], in for loop..... And i am not able to figure out, why it is happening so.... Kindly coreect me, where am i going wrong.

char* concatenate(char *s1, char *s2)
{
   int i,j=0;

   for(i=0; s1[i] != '\0'; i++);
   for(j=0; s2[j] != '\0'; j++)
   {
      s1[i+j] = s2[j];
   }
   s1[i+j] = s2[j];

   return s1;
}

   char *s1 = (char *) malloc(15);;
   char *s2 ;

   s1 = "defds";
   s2 = "abcd";

   s1 = concatenate(s1,s2);
//   printf("\n\n%s\n\n",s1);

Upvotes: 1

Views: 358

Answers (4)

chmod222
chmod222

Reputation: 5722

You don't have to write your own string concatenation, there are already functions in the standard libary to do this task for you! 

#include <string.h>

char *strcat(char *dest, const char *src);
char *strncat(char *dest, const char *src, size_t n);

Upvotes: 2

John Kugelman
John Kugelman

Reputation: 361585

When you do s1 = "rahul"; you are overwriting the memory you just allocated. This line doesn't copy "rahul" to the malloc'ed area, it changes s1 to point to the string constant "rahul" and throws away the pointer to the malloc'ed memory.

Instead, you should use strcpy to copy the string to the malloc'ed area:

// s1 = "rahul";
strcpy(s1, "rahul");

This will fix your call to concatenate since s1 will now point to the correct 15-byte area of memory.

Alternatively, you could eschew the dynamic allocation and allocate+assign the initial string all at once:

char s1[15] = "rahul";

That will allocate 15 bytes on the stack and copy "rahul" into that space. Note a subtlety here. In this case it is in fact correct to use =, whereas it is incorrect when s1 is declared as char *s1.

One important debugging lesson you can learn from this is that when your program crashes on a particular line of code that doesn't mean that's where the bug is. Often you make a mistake in one part of your program and that mistake doesn't manifest itself in a crash until later on. This is part of what makes debugging such a delightfully frustrating process!

Upvotes: 3

James McNellis
James McNellis

Reputation: 355019

s1 = "rahul";

This line does not copy the string "rahul" into the buffer pointed to by s1; it reassigns the pointer s1 to point to the (not modifiable) string literal "rahul"

You can get the desired functionality by using your concatenate function twice:

char *s1 = (char *) malloc(15);

s1[0] = '\0'; // make sure the buffer is a null terminated string of length zero
concatenate(s1, "rahul");
concatenate(s1, "bagai");

Note that the concatenate function is still somewhat unsafe since it blindly copies bytes, much like strcat does. You'll want either to be very sure that the buffer you pass it is large enough, or to modify it to take a buffer length like strncat takes.

Upvotes: 8

Graeme Perrow
Graeme Perrow

Reputation: 57248

In the second piece of code, you allocate a buffer of size 15 and then assign it to s1. Then you assign "rahul" to s1, which leaks the memory you just allocated, and assigns s1 to a 6-byte piece of memory that you likely cannot write to. Change s1 = "rahul"; to strcpy( s1, "rahul" ); and you might have better luck.

I agree with the other answers though, the concatenate function is dangerous.

Upvotes: 1

Related Questions