Error on hand-coded concatenate function (Segmentation fault (core dumped))

Question

I'm trying to make a function that concatenates a, b, and c in a single variable called result.

#include <stdio.h>

const char* concat(char* a, char* b, char* c) {

  char *result;    // Concatenated variable (A + B)
  int i;           // Index of A, B and C
  int ir;          // Index of result

  // result = A
  for(i=0; a[i] != '\0'; i++) {
    ir = i;
    result[ir] = a[i];
  } // → for

  // result = A + B
  ir++;
  for(i = 0; b[i] != '\0'; i++) {
    if (i == 0)
      ir = ir + i;
    result[ir] = b[i];
  } // → for

  // result = A + B + C
  ir++;
  for(i = 0; c[i] != '\0'; i++) {
    ir = ir + i;
    result[ir] = c[i];
  } // → for

  return result;
} // → concatenate()

But when I compile and run with the following block it results in Segmentation fault (core dumped) error. I've already searched about it (What is a segmentation fault?) but the insight didn't come. What am I doing wrong?

int main() {

  char* a = "1234567";
  char* b = "abcdefg";
  char* c = "_______";


  printf("%s", concat(a, b, c));

}

ERROR:

Segmentation fault (core dumped)

Answer 1

The code shown is trying to store data into an undefined buffer (result), and that's not not kosher. SOMEBODY has to allocate the space, either your function, or the caller by passing in a bounded output buffer.

Using the allocate-memory mechanism, you have to get the full size of all the strings in advance so you can allocate the full chunk at once. After allocating the full chunk, copy in the strings one at a time.

char *concat3(const char *a, const char *b, const char *c)
{
    // get the required size in advance
    size_t n = 0;

    for (const char *ap = a; *ap; ap++) n++;
    for (const char *bp = b; *bp; bp++) n++;
    for (const char *cp = c; *cp; cp++) n++;

    char *result = malloc(n + 1);

    char *op = result;

    while (*op = *a++) op++;
    while (*op = *b++) op++;
    while (*op = *c++) op++;

    return result;
}

Also note that the parameters are const char * rather than char * to make sure everybody knows that concat3 - I changed the name - will not write to these input buffers.

However, the return value cannot be const because you have to free it, which I hope you remember to do.

EDIT I added a different version that writes into a bounded output buffer so there's no memory allocation required in the function because the caller is responsible for it:

char *concat(char *obuf, size_t osize, const char *a, const char *b, const char *c)
{
    char *obuf_save = obuf;

    const char *obuf_max = obuf + osize - 1;

    while (obuf < obuf_max  &&  (*obuf = *a++)) obuf++;
    while (obuf < obuf_max  &&  (*obuf = *b++)) obuf++;
    while (obuf < obuf_max  &&  (*obuf = *c++)) obuf++;

    *obuf = 0;

    return obuf_save;
}

The key part is passing the output buffer and size, and it promises not to overwrite the end of the buffer.

Key downside: you can't tell if it truncated or not. Depends on your application if this matters.

Answer 2

You're never initializing result, so it will have a random value, so you've got UB (undefined behavior), which can result in a segfault.

You have to do a malloc for result. And, it needs to have enough space to hold the length of all the concatenated strings.

Here's some updated code. Note that I did not thoroughly check your for loops, but, at first glance they seem to be okay.

But, you have to add an EOS at the end of result because the for loops do not do that.

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

size_t
mystrlen(const char *buf)
{
    const char *bp;

    for (bp = buf;  *bp != 0;  ++bp);

    return bp - buf;
}

#if 0
const char *
concat(char *a, char *b, char *c)
#else
char *
concat(const char *a, const char *b, const char *c)
#endif
{
    char *result;                       // Concatenated variable (A + B)
    int i;                              // Index of A, B and C
    int ir;                             // Index of result

// NOTE/BUG: result is _never_ given a value
#if 1
    size_t totlen = 0;

    // we need to determine the total length of the result string
    totlen += mystrlen(a);
    totlen += mystrlen(b);
    totlen += mystrlen(c);

    result = malloc(totlen + 1);
    *result = 0;
#endif

    // result = A
    for (i = 0; a[i] != '\0'; i++) {
        ir = i;
        result[ir] = a[i];
    }

    // result = A + B
    ir++;
    for (i = 0; b[i] != '\0'; i++) {
        if (i == 0)
            ir = ir + i;
        result[ir] = b[i];
    }

    // result = A + B + C
    ir++;
    for (i = 0; c[i] != '\0'; i++) {
        ir = ir + i;
        result[ir] = c[i];
    }

    result[totlen] = 0;

    return result;
}

---

Edit: This was my original post. But, as others have pointed out, it was not the real issue.

So, disregard this section altogether!

I'm leaving it in because several responders here also misread the intent of your function at first glance. So, you may wish to add more comments to clarify the use of arguments and return value.

When you do:

char* a = "1234567";

You are creating a pointer that points to a string literal. On most/many systems, the literal will be placed in read only memory.

So, you will segfault on a protection exception [because you're trying to write to read only memory].

Also, the target buffer needs to be large enough to contain the size of all the concatenated strings. As you have it, a is too short/small.

You could try:

char a[100] = "1234567";

This creates a writable buffer of length 100 that is initialized with the string. But, you'll still have extra space to accomodate the concatenated strings [up to a total length of 100].