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;
}

Sergey Kalinichenko · Accepted Answer · 2014-08-17 16:25:52Z

3

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.

edited Aug 17, 2014 at 16:25

answered Aug 17, 2014 at 16:00

Sergey Kalinichenko

729k85 gold badges1.2k silver badges1.6k bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Abe Fehr Over a year ago

You're right about eStr, and I'll fix that immediately. Running your code still fails in cmd though, and I presume it has to do with the stderr output in the demo you linked to

Sergey Kalinichenko Over a year ago

@AbeFehr Ah, I did not notice the stderr. Let me check what's going on there. Output like this usually means that you have written to an address before what's been allocated to you.

Abe Fehr Over a year ago

It was my memmove, as pointed out by BLUEPIXY's answer. Thanks to you both!

BLUEPIXY · Accepted Answer · 2014-08-17 16:11:22Z

2

should be

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

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

answered Aug 17, 2014 at 16:11

BLUEPIXY

40.2k7 gold badges37 silver badges73 bronze badges

Collectives™ on Stack Overflow

Managing memory when returning string in C

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related