Where am I going wrong with my memory management?

Question

https://gist.github.com/macarr/b49c0097666a613f639c4eab931f31d4

I'm making a little app in C that's supposed to connect to the ecobee API. Ignore for the moment that this is dumb/terrible/why god are you using C as a REST API client, it's a personal project for the fun of it.

I'm currently running into a problem with memory management. I've annotated the provided gist with comments and snipped out code that I don't believe is related. Basically, the app works exactly as expected until I reach getTokens. Then the following code freaks the heck out:

struct authorizations getTokens(char* apiKey, char* authCode) {
  char* body = "grant_type=ecobeePin&code=";
  body = concat(body, authCode);
  printf("%s!!!!!!!!!!\n", body); //as expected
  concat2(body, "&client_id=");
  printf("%s!!!!!!!!!!\n", body); //as expected
  concat2(body, apiKey);
  printf("%s!!!!!!!!!!\n", body); //garbage
  ...

See the gist for the function of concat and concat2.

char* concat(const char *s1, const char *s2)
{
    char* result;
    result = malloc(strlen(s1)+strlen(s2)+1);//+1 for the zero-terminator
    if(!result) {
        exit(1);
    }
    strcpy(result, s1);
    strcat(result, s2);
    printf("CONCAT1: %s\n", result);
    return result;
}

void concat2(char *s1, const char *s2) {
    char temp[strlen(s1)];
    strcpy(temp, s1);
    printf("%s:%s\n", temp, s2);
    s1 = realloc(s1, strlen(temp) + strlen(s2) + 1);//+1 for the null terminator
    if(!s1) {
        exit(1);
    }
    strcpy(s1, temp);
    strcat(s1, s2);
    printf("CONCAT2: %s\n", s1);
}

At the end of my function, I free(body), which kills the application because apparently body has already been freed. I guess one of my reallocs aren't working or something?

What confuses me most is that when I was working with bad data two days ago (making invalid calls to the api and just pulling info from the errors to populate the requests going forward - I didn't have a login set up then), everything was working fine. Once I started getting real data, the application started "stack smashing". This is as far as I was able to push it last night.

Additionally, any general advice on where I'm going wrong in regards to string concatenation and pointer manipulation would be welcome. Assume I've already heard why I shouldn't be using C as a REST API client

Answer 1

As pointed out by @n.m. and @BLUEPIXY, there is a serious problem with concat2. Assuming you don't want to spend any more time figuring it out for yourself, spoilers follow...

The primary issue is that concat2 dutifully takes the buffer pointed to by s1, reallocs it to ensure it's big enough for the concatenation, storing the pointer to the brand new resized buffer in s1. It then does the string concatenation into that new buffer, and then -- when the function ends -- it throws the all-important new pointer value in s1 away. After the first call to concat2, your getTokens function is under the mistaken impression that the buffer is still located at body, but the location might very well have been changed!

(Depending on how the allocator works on your particular platform, it may or many not actually change, depending on the old and new sizes and allocator details, but you need to assume it might have changed.) So, if you rewrite concat2 as:

char* concat2(char *s1, const char *s2) {
    char temp[strlen(s1)];
    strcpy(temp, s1);
    s1 = realloc(s1, strlen(temp) + strlen(s2) + 1);//+1 for the null terminator
    if(!s1) {
        exit(1);
    }
    strcpy(s1, temp);
    strcat(s1, s2);
    return(s1);  /* IMPORTANT: new buffer location! */ 
}

and modify your concat2 calls so they look like:

body = concat2(body, "&client_id=");
...
body = concat2(body, apiKey);

you will find that things work better.

Another point to note: realloc already copies the previous contents of the buffer to the new buffer (up to its originally malloced size; any added memory will be uninitialized), so you don't need to use the temp buffer and extra copying at all and concat2 can be simplified to:

char* concat2(char *s1, const char *s2) {
    s1 = realloc(s1, strlen(s1) + strlen(s2) + 1);//+1 for the null terminator
    if(!s1) {
        exit(1);
    }
    /* now s1 points to the original string at the beginning
     * of a sufficiently large buffer
     */
    strcat(s1, s2);
    return(s1);
}

Answer 2

Okay, you have a major problem here:

s1 = realloc(s1, strlen(temp) + strlen(s2) + 1);

This can potentially change the value of s1, which is promptly lost as soon as you exit the function. You do not return this new pointer value back to the caller, so it's using the old, now invalid pointer value. Hence the lossage.

You need to make the changed value of s1 available to the caller, meaning you must pass a pointer to it:

void concat2(char **s1, const char *s2) {
    /**
     * You do not need to preserve the contents of s1 here - if successful,
     * realloc will copy the contents of s1 to the new memory; if not, 
     * it will leave the existing contents in place.  
     * 
     * Because realloc can return NULL on failure, you should *not*
     * assign the result back to the original pointer, but instead
     * assign it to a temporary; that way, if realloc does fail, you
     * don't lose the reference to the previously allocated memory
     */
    char *tmp = realloc(*s1, strlen(*s1) + strlen(s2) + 1);//+1 for the null terminator
    if(!tmp) {
        // handle realloc error
    }
    *s1 = tmp;
    strcat(*s1, s2);
    printf("CONCAT2: %s\n", *s1);
}

You'd then call this as

concat2(&body, "&client_id=");
...
concat2(&body, apiKey);