Managing memory when returning string in C

Question

I have a CGI application I've been writing in Visual Studio Express 2013, but I've encountered a scenario where the program fails when the string passed into a function is more than 31 bytes. I would try to debug it myself, but everything works fine in Visual Studio Debugger, it's only in Command Prompt where I see the error.

I believe this to be the way that I've allocated(or not allocated) the memory. I have a function which takes in a string and returns the decoded string.

I've stripped everything down to just that function and main so you can see two cases, one which works in CMD and one which fails.

Here's the file:

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

/*
* Function: urlDecode
* Purpose:  Decodes a web-encoded URL
* Input:    const char* str - the URL to decode
* Output:   char* - the decoded URL
*/
char *urlDecode(const char *str) {
    int d = 0; /* whether or not the string is decoded */

    char *dStr = malloc(strlen(str) + 1);
    char *eStr = malloc(2); /* a hex code */

    strcpy(dStr, str);

    while (!d) {
        d = 1;
        int i; /* the counter for the string */
        int j = strlen(dStr);

        for (i = 0; i<j; ++i) {
            if (dStr[i] == '%') {
                if (dStr[i + 1] == 0)
                    return dStr;

                if (isxdigit(dStr[i + 1]) && isxdigit(dStr[i + 2])) {
                    d = 0;

                    //combine the next two numbers into one
                    eStr[0] = dStr[i + 1];
                    eStr[1] = dStr[i + 2];

                    //convert it to decimal
                    long int x = strtol(eStr, NULL, 16);

                    //remove the hex
                    memmove(&dStr[i], &dStr[i + 2], strlen(dStr) - 1);

                    dStr[i] = x;
                    j = j - 2;
                }
            }
        }
    }
    free(eStr);
    return dStr;
}

int main(void)
{
    //this one runs fine from command prompt
    char *test1 = "s=W3%20%3A%20SVC&action=stop";
    printf("%s\n", test1);
    char *decoded1 = urlDecode(test1);
    printf("%s\n", decoded1);
    free(decoded1); //and I can even free the memory

    //this one prints in command prompt, but the program crashes immediately after
    char *test2 = "service=W3%20%3A%20SVC&action=stop";
    printf("%s\n", test2);
    char *decoded2 = urlDecode(test2);
    printf("%s\n", decoded2);
    //when I comment this free out, it debugs fine in VS, but still fails in cmd
    //this is the reason I believe it's a memory error
    //free(decoded2); 

    system("PAUSE"); //so I can see what's happening

    return 0;
}

Answer 1

You failed to null-terminate eStr, and you did not allocate enough memory for it: you need three characters, not two.

Since eStr is so short, consider making it a stack variable, rather than allocating it on the dynamic store:

char eStr[] = "00";

This would allocate enough space, and make it unnecessary to free the pointer at the end of your function.

Another issue is memmove: it looks like your indexes are off. You could fix it, but it's much easier to avoid memmove altogether: rather than making substitutions in place, use str as your source, and dStr as your destination:

char *urlDecode(const char *str) {
    int d = 0; /* whether or not the string is decoded */

    char *dStr = malloc(strlen(str) + 1);
    char *ret = dStr;
    char eStr[] = "00";

    strcpy(dStr, str);

    while (!d) {
        d = 1;
        int i; /* the counter for the string */
        int j = strlen(dStr);

        for (i = 0; i<j; ++i) {
            if (str[i] == '%') {
                if (str[i + 1] == 0) {
                    break;
                }
                if (isxdigit(str[i + 1]) && isxdigit(str[i + 2])) {
                    d = 0;

                    //combine the next two numbers into one
                    eStr[0] = str[i + 1];
                    eStr[1] = str[i + 2];

                    //convert it to decimal
                    long int x = strtol(eStr, NULL, 16);

                    *dStr++ = x;
                }
            } else {
                *dStr++ = str[i];
            }
        }
    }
    *dStr = 0;
    return ret;
}

Demo.

Answer 2

should be

char *eStr = calloc(3,1); /* a hex code */

memmove(&dStr[i+1], &dStr[i + 3], strlen(&dStr[i+3])+1 );