0

When trying to reallocate memory I crash when using this code:

//value of i is currently 4096
while((c = recv(sock, htmlbff + q, MAXDATASIZE, 0)) > 0)
{
    if((i - q) < MAXDATASIZE)
    {
            i *= 2;
            if(!(tmp = realloc(htmlbff, i)))
            {
                free(htmlbff);
                printf("\nError! Memory allocation failed!");
                return 0x00;
            }
            htmlbff = tmp;
    }
    q += c;
}

it crashes because of a memory leak.... HOWEVER the following code does not crash:

while((c = recv(sock, htmlbff + q, MAXDATASIZE, 0)) > 0)
{
    if((i - q) < MAXDATASIZE)
    {
        i *= 2;
        if(!(tmp = realloc(htmlbff, i)))
        {
            free(htmlbff);
            printf("\nError! Memory allocation failed!");
            return 0x00;
        }
    }
    htmlbff = tmp;
    q += c;
}

How can moving htmlbff = tmp; outside of the if statement be fixing this problem? It doesn't seem to set htmlbff to tmp when inside the if statement... I am extremely confused.

Keith Miller
  • 1,337
  • 1
  • 16
  • 32
  • I think you may have memory corruption somewhere else that eventually crashes here. Assuming that the '?' is just a typo in your first example and that the `htmlbff` pointer is allocated by the time you call `realloc()`, this seems to be ok. What makes it work or crash is not the fact that you moved that line of code - it's just that the heap is possibly corrupted by the time you call realloc() here. – xxbbcc Aug 24 '12 at 17:15
  • Typo? `if((i - q) ? MAXDATASIZE)` – Jo So Aug 24 '12 at 17:15
  • yes typo sorry corrected code appended, I was thinking it might be corrupted memory elsewhere but why on earth does it NOT crash when I move that line, but it does when it says in it's intended location? – Keith Miller Aug 24 '12 at 17:16
  • Just a guess: `i` and `q` are unsigned (probably `size_t`). When `q` becomes larger than `i`, the result of the subtraction overflows and the condition `< MAXDATASIZE` is not met. – Greg Inozemtsev Aug 24 '12 at 17:16
  • 1
    "it crashes because of a memory leak" -- a memory leak is not a cause of a crash, and "crash" is not sufficiently descriptive. What does it *really* do? Also, moving htmlbff = tmp outside the loop is not the only difference between these pieces of code, and the other difference is a syntax error, so what's the *real* code? – Jim Balter Aug 24 '12 at 17:17
  • @KeithMiller moving that assignment cannot possibly fix the issue - it's likely a problem by itself since you're overriding `htmlbff` with `tmp` which could be uninitialized outside your `if()`. Heap corruption is the most likely cause for your problem (assuming your variables are otherwise correctly initialized). – xxbbcc Aug 24 '12 at 17:18
  • How odd... I guess it must be heap corruption then... I guess the best course of action will be to back track my code until I see what is amiss. – Keith Miller Aug 24 '12 at 17:23
  • If you're using Visual Studio, watch out for heap warnings (in the Output window) when your program quits - if you have any, that indicates severe heap problems. Do a Google search for tracking down heap problems using the debug heap in this case. (I'm not familiar with Linux to give advice there.) – xxbbcc Aug 24 '12 at 17:25
  • I'm using Pelles C for windows, but it's still strange... if I set i initially to a rather large number the issue seems to subside... but then again I'm never encountering the if statement in that case I'm just a bit upset with this situation. – Keith Miller Aug 24 '12 at 17:28
  • At any rate my debugger outputs: `ODS: HEAP[Function Pointer.exe]:` `ODS: Heap block at 00000000004810B0 modified at 00000000004820CF past requested size of 100f` – Keith Miller Aug 24 '12 at 17:30
  • Just tell us where you have your declaration/initialization of `tmp`. Or better, add it to your post – xQuare Aug 24 '12 at 17:32
  • Shouldn't `q += c;` go before the outer `if()` to avoid a buffer overflow? – alk Aug 24 '12 at 17:33
  • `char *html, *htmlbff, *req, *szbff, *tmp;` Declared at top of function html is mallocd to 4096 bytes before the loop `if(!(htmlbff = malloc(i)))` – Keith Miller Aug 24 '12 at 17:34
  • `if((i - q) < MAXDATASIZE) { ` print something to the screen at this point to see if the condition is true and then tell us – xQuare Aug 24 '12 at 17:37
  • hmm it looks like the debugger is actually my best bet, I will post the solution when I get it, I know it's because of poor coding somewhere earlier. – Keith Miller Aug 24 '12 at 17:39
  • @papergay did that myself already statement is indeed true. the debugger is showing me some heap problems though, as mentioned earlier I will use the debugger (first time) and report back. Thank you – Keith Miller Aug 24 '12 at 17:39
  • For the ease of debugging you might like to set the initial value of i = MAXDATASIZE = 1 and then simply count. If the problem is solved you can go back to setting i to any larger value and MAXDATASIZE to any value < the initial value for i. Try following the paradigm to keep things as simple as possible. – alk Aug 24 '12 at 17:43
  • what? I don't understand what you mean – Keith Miller Aug 24 '12 at 17:55
  • I took out all the printf's and now it works fine ? – Keith Miller Aug 24 '12 at 17:58
  • Removing printf() is not the solution. If the debugger gives you heap warnings like you showed, you're corrupting the heap - you need to find the place where you write past the end of a heap block or where you possibly double-free some memory. – xxbbcc Aug 24 '12 at 18:16
  • So you surely won't mind telling us how you solved the problem, will you? – alk Aug 24 '12 at 18:31

2 Answers2

3
/*
 * I assume these exist, and more or less fit the requirements described.
 * They don't have to be these specific numbers, but they do need to have
 * the specified relationships in order for the code to work properly.
 */
#define MAXDATASIZE 4096    /* any number here is ok, subject to rules below */
int i = 4096;               // i >= MAXDATASIZE, or the first recv can trigger UB
char *htmlbff = malloc(i);  // ITYK you can't realloc memory that wasn't malloc'd
int q = 0;                  // q <= i-MAXDATASIZE

/* maybe other code */

while((c = recv(sock, htmlbff + q, MAXDATASIZE, 0)) > 0)
{
    /*
     * You've already just read up to MAXDATASIZE bytes.
     * if (i-q) < MAXDATASIZE, then **your buffer is already too small**
     * and that last `recv` may have overrun it.
     */
    if((i - q) < MAXDATASIZE)
    {
        ... reallocate htmlbff ...
    }
    /* Then you bump q...but too late.  lemme explain in a sec */
    q += c;
}

Let's say you recv 4096 bytes twice in a row. What happens is:

  1. The first read reads 4096 bytes, starting at htmlbff + 0.
  2. Since q is still 0, i - q == 4096, so no allocation is done.
  3. q is bumped up by 4096.
  4. The second read gets 4096 bytes, starting at htmlbff + 4096. But wait, since we didn't resize it last iteration, htmlbff is only 4096 bytes big, and the entire read spills out of the buffer!
  5. If you're lucky, the overrun causes a segfault and the program dies. If you're not, then the CPU just soldiers on, and any behavior from here on is undefined. There's very little point in even diagnosing further issues at this point, since $DEITY knows what the code just trashed.

Try this instead...

while((c = recv(sock, htmlbff + q, MAXDATASIZE, 0)) > 0)
{
    /* **First**, bump `q` past the stuff you just read */
    q += c;

    /*
     * **Now** check the buffer.  If i-q is too small at this point, the buffer is
     * legitimately too small for the next read, and also hasn't been overrun yet.
     */
    if((i - q) < MAXDATASIZE)
    {
        /* This temp pointer **really** should be limited in scope */
        char *double_sized;

        /* Note that otherwise, i'm using the "broken" resize code.
         * It should work fine.
         */
        i *= 2;
        if(!(double_sized = realloc(htmlbff, i)))
        {
            free(htmlbff);
            printf("\nError! Memory allocation failed!");
            return 0x00;
        }
        htmlbff = double_sized;
    }
}
cHao
  • 84,970
  • 20
  • 145
  • 172
  • I need to take the time to thank you for breaking that down for me, you really helped me a lot! I immensely appreciate it! – Keith Miller Aug 26 '12 at 22:04
1

You CAN NOT receive data using a buffer smaller than data and then realloc it to fit the bigger data.

You need rewrite your code:

buffer = malloc(MAXDATASIZE)
size_t received = 0    
while ((size_t sz = recv(sock, buffer+received, MAXDATASIZE-received, 0) > 0)
{
       received += sz 
}
buffer = realloc(buffer, received);

I changed the variables names and was verbose for clarification. Your code's weird behavior probably is random and both codes are wrong. You can learn more here: http://en.wikipedia.org/wiki/C_dynamic_memory_allocation

cHao
  • 84,970
  • 20
  • 145
  • 172
olivecoder
  • 2,858
  • 23
  • 22
  • 3
    Yes you can; that's precisely what `realloc` is for. See http://en.cppreference.com/w/c/memory/realloc – ecatmur Aug 24 '12 at 19:59
  • What @ecatmur said: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html. – InternetSeriousBusiness Aug 24 '12 at 20:34
  • This isn't the OP's problem though. The htmlbuf buffer is already bigger than MAXDATASIZE. After doing one read, he's seeing if the _next_ read might have issues because the remaining part of the buffer is too small, and enlarging it enough to handle it. – Collin Aug 24 '12 at 20:40
  • Are you wrong @Collin! the recv function does not do any realloc but overwrite memory! It is as C language works. See my code in: http://stackoverflow.com/questions/12116129/realloc-expansion-after-memory-usage#comment16198966_12116129 – olivecoder Aug 24 '12 at 20:54
  • @olivecoder I know this. We don't see the original malloc of htmlbuf, but it's larger than `MAXDATASIZE` (presumably). After the first read, the i - q checks to see if `htmlbuf` still has at least `MAXDATASIZE` space left. If not, it doubles the size of `htmlbuf`, making sure the _next_ call to `recv` will have enough space to hold the read. – Collin Aug 24 '12 at 20:56
  • Well, if you are getting a crash I dont will presume that MAXDATASIZE is correct. – olivecoder Aug 24 '12 at 21:03