Why does the code leak memory?

Question

I am wondering why this little peace of code is leaking memory? It is flipping my image horizontally and returning the struct Image to my main file.

void turnImg(Image *img) {

    Image *tmp = (Image*)malloc(sizeof(Image));

    //swapping size between height and width
    tmp->height = img->height;
    img->height = img->width;
    img->width = tmp->height;

    //The loop is swapping every pixel from the "last" pixel to the "first" into a tmp
    tmp->pixels = (Pixel**)malloc(sizeof(Pixel*)*img->height);
    for (unsigned int i = 0; i < img->height; i++) {
        tmp->pixels[i] = (Pixel*)malloc(sizeof(Pixel)*img->width);
        for (unsigned int j = 0; j < img->width; j++) {
            tmp->pixels[i][j] = img->pixels[img->width - 1 - j][img->height - 1  - i];
        }
    }

    for (unsigned i = 0; i < img->height; i++) {
        free(img->pixels[i]);
    }
    free(img->pixels);
    //tmp gives back the pixels to img, but they are now flipped
    img->pixels = tmp->pixels;

    free(tmp);
}

Nothing should be wrong in the main file because all of my other functions are doing fine... But here is the peace of the main which is sending and returning the struct to the function:

case 5:
        //Flipping the image on its diagonal.
        printf("Flipping the image on its diagonal...");
        turnImg(&plain);
        printf("Image flipped.\n");
        break;

And the main file ends with:

for (unsigned int i = 0; i < plain.height; i++) {
    free(plain.pixels[i]);
}
free(plain.pixels);

getchar();
return 0;

However, what I have noticed is that the height and width swap is a part of the problem, but I do not how I will be able to do this without a swap.

You are using `malloc` on of `tmp->pixels[i]`, but at the end, you are doing `free(img->pixels[i])` instead of `free(tmp->pixels[i])`. When you free `tmp` at the end, all of those allocated pixels on `tmp` are being lost. Might that be what the issue is? — Random Davis, Feb 07 '17 at 19:28
You should read [Do I cast the result of malloc?](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Stargateur, Feb 07 '17 at 19:31
Of course swapping the pixel values mirrors the image. Try it on paper with a 3x2 pixel array with values say `a` to `f`. — Weather Vane, Feb 07 '17 at 19:32
Oh sorry, I am embarrassed... I uploaded a change which I made when I was testing some stuffs... I edited it to the original again. — Henke, Feb 07 '17 at 19:37
@Stargateur I have read that link "Do I cast the result of malloc?". Some answers say don't; some say you do; others say "it doesn't matter". So, which one to follow? ;) — P.P, Feb 07 '17 at 19:41
@P.P. I force nobody but one has 1500 votes who don't cast and 210 votes who cast... choice your side :p — Stargateur, Feb 07 '17 at 19:45
@RandomDavis He is replacing the supplied image's pixels with the new ones. So of course he wants to free the old pixels and not the new ones. Look at the code again, particularly this line `img->pixels = tmp->pixels;`. — David Schwartz, Feb 07 '17 at 19:46
Why not swap in place without allocating a new set of buffers? — Ry-, Feb 07 '17 at 19:47
@Henke How did you determine that this code leaks memory? If you eliminate the call to `turnImg`, does that change anything? — David Schwartz, Feb 07 '17 at 19:47
@DavidSchwartz Well, I accidentally posted a wrong code when Davis was answering, so that is my fault. — Henke, Feb 07 '17 at 19:49
Here's a tip to make the code easier to understand: Set two variables, `origHeight` and `origWidth` immediately on entry. Only use those two in the function in all the loops and allocations. Set the returned image's width and height from those. That will make it much easier to see if you used the correct values in the correct places. This code is hard to follow because of the swaps and assignments. — David Schwartz, Feb 07 '17 at 19:51
@DavidSchwartz Well I am using _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF); in my debugger. And no other functions are leaking. Only this flipping function. And when I ignore the call I do not get any leaks. — Henke, Feb 07 '17 at 19:51
Okay, then see my comment above. Maybe you got the heights and widths mixed up -- it's very hard to tell through the swaps and assignments. — David Schwartz, Feb 07 '17 at 19:52
when asking a question about a run time problem: post code that cleanly compiles (and is short) post the actual input, post the actual output — user3629249, Feb 09 '17 at 11:24
to reverse an image, left to right (horizontally) There is no need to use any heap memory allocation. There is no need for any temporary buffers beyond the actual width of a pixel (and even that can be avoided by using 'exclusive or' statements. Strongly suggest you re-think your algorithm. And remember that the actual number of bytes, horizontally, will be a multiple of 4, so there can be up to 3 dummy bytes on the end of each pixel row. — user3629249, Feb 09 '17 at 11:27
For us to easily debug your problem, the definition of the struct `image` needs to be posted — user3629249, Feb 09 '17 at 11:29

jxh · Accepted Answer · 2017-02-07T19:57:36.833

3

You have modified img by swapping its height and width.

However, when you are freeing up img to make room for the tmp pixels, you are freeing img following the tmps height. That is, you are using imgs new height rather than its original height.

To fix, change your loop to free using imgs width (which is its old height).

    for (unsigned i = 0; i < img->width; i++) {
        free(img->pixels[i]);
    }

edited Feb 07 '17 at 19:57

answered Feb 07 '17 at 19:43

jxh

69,070
8
110
193

@DavidSchwartz He replaces `img->pixels` with `tmp->pixels` after he frees `img->pixels`. He doesn't swap them before he frees them. Thus, he's freeing the wrong ones, right? – Random Davis Feb 07 '17 at 19:48
Yes, it is working now. Thank you! I can not believe it was so simple... – Henke Feb 07 '17 at 20:04
Yes, it would probably have led me to this solution eventually :) – Henke Feb 07 '17 at 20:06

Why does the code leak memory?

1 Answers1