Weird behaviour of C strcpy

Question

I wrote a tiny program in C. It compile perfectly and works, when I compile it through my Makefile and clang, however, in Xcode this function behaves not like it should (or it behaves like it should and clang is ignoring it).

size_t getUrlForArgAndPlatform(char **dest, const char *arg, const char *platform) {
    int isLinux = strcmp(platform, "Linux");
    int isOSX = strcmp(platform, "Darwin");

    char *platformUrlDelimiter = malloc(6 + 1);
    if (isLinux == 0) {
        strcpy(platformUrlDelimiter, "linux");
    } else if (isOSX == 0) {
        strcpy(platformUrlDelimiter, "osx");
    } else {
        strcpy(platformUrlDelimiter, "common");
    }

    int length = (int) strlen(kBaseUrl);
    length += strlen(platformUrlDelimiter);
    length += strlen(arg);
    length += 5;

    char *buffer = (char *) malloc(length);
    if (buffer == NULL) {
        exit(EXIT_FAILURE);
    }
    strcpy(buffer, kBaseUrl);
    strcat(buffer, "/");
    strcat(buffer, platformUrlDelimiter);
    strcat(buffer, "/");
    strcat(buffer, arg);
    strcat(buffer, ".md");

    *dest = malloc(strlen(buffer) + 1);
    strcpy(*dest, buffer);

    free(platformUrlDelimiter);
    free(buffer);

    return strlen(buffer) + 1;
}

It works 4 times out of 10. In the other 6 times, Xcode is telling me it's failing at strcpy(*dest, buffer) with a SIGBRT. If I take a look at the debugger, I see that buffer contains the same string twice. Why?

Answer 1

The size you have calculated for buffer is not quite correct:

int length = (int) strlen(kBaseUrl);
length += strlen(platformUrlDelimiter);
length += strlen(arg);
length += 5;

The last part should be '+6' because you need space for 2 times "/", for ".md" AND for the terminating NUL.

Answer 2

(The incorrect length has already been mentioned.)

There is a lot of pointless copying going on in your code. Since you don't modify platformUrlDelimiter, there is no need to copy it twice. This is much simpler:

const char *platformUrlDelimiter = "common";
if (isLinux == 0) {
    platformUrlDelimiter = "linux";
} else if (isOSX == 0) {
    platformUrlDelimiter = "osx";
}

And remove the call to free (platformUrlDelimiter).

Every call to strcat() (probably) has to needlessly loop through the buffer. I would write:

int length = strlen(kBaseUrl)
           + strlen(platformUrlDelimiter)
           + strlen(arg)
           + 6;

*dest = malloc(length);
if (*dest) {
  sprintf (*dest, "%s/%s/%s.md", kBaseUrl, platformUrlDelimiter, arg);
} else {
  /* Error */
}

return length;

Now there is no need to use buffer at all.

Also, there's a real bug right at the end of your code:

free(buffer);

return strlen(buffer) + 1;

You must not use the value of buffer in any way after it has been freed.