Custom codebase help

Posted by Zalethon on Sun 09 Dec 2007 03:46 PM — 5 posts, 25,908 views.

Zalethon USA Sun 09 Dec 2007 03:46 PM #0

Seems like kind of a dumb place to ask, but the only one on the forum that fits :P

I was working on a function to at least temporarily fix some MXP problems, and I got it working except that there's a memory leak in the function I'm not seeing. (This is for a custom codebase, not PennMUSH or anything)


char *smash_mxp ( char *str )
{
        int i, j = 0;
        char *str1;
        str1 = malloc(sizeof(str));
        memmove(str1,str,strlen(str)+1);

        for (i = 0; str1[i] != '\0';i++)
        {
                switch (str1[i])
                {
                        case '<':
                        str[j] = '&';
                        str[j +1] = 'l';
                        str[j +2] = 't';
                        str[j +3] = ';';
                        j +=3;
                        break;
                        case '>':
                        str[j] = '&';
                        str[j +1] = 'g';
                        str[j +2] = 't';
                        str[j +3] = ';';
                        j +=3;
                        break;
                        case '&':
                        str[j] = '&';
                        str[j +1] = 'a';
                        str[j +2] = 'm';
                        str[j +3] = 'p';
                        str[j +4] = ';';
                        j +=4;
                        break;
                        default:
                        str[j] = str1[i];
                        break;
                }
                j++;
        }
        free(str1);
        return str;
}

I'm aware there's probably a dozen better ways to go about doing this, but before we get into that I'd like to understand why the allocated memory isn't being freed.. Then, by all means :P

I'm also aware that that will break things for people whose clients don't support MXP, but it's only temporary, really. I could always check for support, if need be.

Nick Cash USA Sun 09 Dec 2007 04:22 PM #1

Well, I can see one problem.


        str1 = malloc(sizeof(str));

This is a bad allocation. You are allocating four bytes (the size of a pointer) to str1. I think your original intention was to allocate enough space to copy the string, which is somewhat different:


        str1 = malloc((strlen(str)+1)*sizeof(char));

Supposing the compiler will accept that... I'm so used to using C++'s string data type and new operator I forget my malloc syntax.

So if that code was actually working consider yourself lucky. A few other comments though. This function adds 4-5 characters per preexisting special character. You may very well need to resize your base string to accomadate this, as your function currently assumes it won't overrun its bounds. A good way to do this might be to scrap your incoming string and return your newly allocated one. This has other implications in the calling functions, however.

Zalethon USA Sun 09 Dec 2007 04:40 PM #2

Quote:
So if that code was actually working consider yourself lucky. A few other comments though. This function adds 4-5 characters per preexisting special character. You may very well need to resize your base string to accomadate this, as your function currently assumes it won't overrun its bounds. A good way to do this might be to scrap your incoming string and return your newly allocated one. This has other implications in the calling functions, however.

It actually was working. I had it adding the characters to str1 before, and returning str1, but it didn't work for some dumb reason. I might have just screwed it up, I'll try again in a bit...

Nick Gammon Australia Forum Administrator Sun 09 Dec 2007 07:18 PM #3

Quote:

... but it didn't work for some dumb reason ...

Whiteknight explained the reason. You were making an existing string longer, which would have corrupted memory past the end of it.

He is also correct about changing the malloc to use strlen and not sizeof.

Even then, you need to malloc the new string to allow for the extra stuff you are adding. Take a look at the way I did it here:

http://www.gammon.com.au/mushclient/addingservermxp.htm

In that post, I first calculate how much extra memory is needed (by looking for <, >, and & characters). Then I allocate the correct extra amount of memory. Then I copy the text.

Nick Cash USA Tue 11 Dec 2007 04:48 PM #4

Quote:


str1 = malloc((strlen(str)+1)*sizeof(char));

I should note that the char data type is typically (I think guaranteed even) to be one byte, so you could rewrite the line to:


str1 = malloc(strlen(str)+1);

Which is a bit less confusing.

Of course you will, like Nick said, want to calculate how much extra memory you will need first.