Memory leak

[iamthekris]

I am writing a program in which one of the features is a screen capture process which then saves the captured image to a bmp file. the screen capture aspect and the save to bmp are both in seperate files which pass parameters from screencap to bmp to get the job done. After an image file is created on the computer, the program jumps up from under 1000 kb of memory use to over 100,000 kb of memory use, when another pic is taken it jumps up to above 200,000 and so on, which seems like a memory leak. My question is, is there a way to clear the memory that these two functions are creating from the main function as to create a new instance the next time this process is called? If not i guess i can post the code and see if anyone knows what is going wrong, thanks.

http://i.imgur.com/2f9wB.png

[Athar]

Well, you're forgetting to delete something that you no longer need, which is a memory leak.
For anything more specific, you'll have to post the code.

[zwA4jE8b]

I guess it depends on what operations your functions are performing. If you are using the new keyword (especially multiple times) and not deleting the allocated memory on the heap then it will build up

[iamthekris]

These are both codes that someone else wrote and i changed a few things to make it more to my liking. The main calls on the first code which then calls on the second code. First one captures the screen, second one saves it as a bmp.

#include <windows.h>
#include <iostream>
#include <stdio.h>
#include <time.h>
#include <string>
using namespace std;

 
bool SaveBMPFile(char *filename, HBITMAP bitmap, HDC bitmapDC, int width, int height);
 
bool ScreenCapture(int x, int y, int width, int height, char *filename){
   // get a DC compat. w/ the screen
   HDC hDc = CreateCompatibleDC(0);    
 
   // make a bmp in memory to store the capture in
   HBITMAP hBmp = CreateCompatibleBitmap(GetDC(0), width, height);   
 
   // join em up
   SelectObject(hDc, hBmp);   
 
   // copy from the screen to my bitmap
   BitBlt(hDc, 0, 0, width, height, GetDC(0), x, y, SRCCOPY);  
 
   // save my bitmap
   bool ret = SaveBMPFile(filename, hBmp, hDc, width, height); 
 
   // free the bitmap memory
   DeleteObject(hBmp);  
 
   return ret;
   }
 
extern int cap()
{
	
  time_t rawtime;
  struct tm * timeinfo;
  char buffer [80];
  
  

  time ( &rawtime );
  timeinfo = localtime ( &rawtime );

  strftime (buffer,80,"%b-%d-%y--%H-%M-%S.bmp",timeinfo);
  puts (buffer);
  char * F = buffer;

  
  

  ScreenCapture(0, 0, GetSystemMetrics(SM_CXVIRTUALSCREEN), GetSystemMetrics(SM_CYVIRTUALSCREEN), F);
  
   
   return 0;
}

//---------------------------------------------------------------------------
//    Based on:
//    Screenshot - Author: Michael Fötsch; Date: May 31, 2000
//
//---------------------------------------------------------------------------

//---------------------------------------------------------------------------
#include <windows.h>
#include <string>
#include <iostream>
#include <stdio.h>
using namespace std;

// Helper function to retrieve current position of file pointer:
inline int GetFilePointer(HANDLE FileHandle){
	return SetFilePointer(FileHandle, 0, 0, FILE_CURRENT);
	}
//---------------------------------------------------------------------------

// Screenshot
//    -> FileName: Name of file to save screenshot to
//    -> lpDDS: DirectDraw surface to capture
//    <- Result: Success
//
extern bool SaveBMPFile(char *filename, HBITMAP bitmap, HDC bitmapDC, int width, int height){
    bool Success=false;
    HDC SurfDC=NULL;        // GDI-compatible device context for the surface
    HBITMAP OffscrBmp=NULL; // bitmap that is converted to a DIB
    HDC OffscrDC=NULL;      // offscreen DC that we can select OffscrBmp into
    LPBITMAPINFO lpbi=NULL; // bitmap format info; used by GetDIBits
    LPVOID lpvBits=NULL;    // pointer to bitmap bits array
    HANDLE BmpFile=INVALID_HANDLE_VALUE;    // destination .bmp file
    BITMAPFILEHEADER bmfh;  // .bmp file header

    // We need an HBITMAP to convert it to a DIB:
    if ((OffscrBmp = CreateCompatibleBitmap(bitmapDC, width, height)) == NULL)
        return false;

    // The bitmap is empty, so let's copy the contents of the surface to it.
    // For that we need to select it into a device context. We create one.
    if ((OffscrDC = CreateCompatibleDC(bitmapDC)) == NULL)
		return false;

    // Select OffscrBmp into OffscrDC:
    HBITMAP OldBmp = (HBITMAP)SelectObject(OffscrDC, OffscrBmp);

    // Now we can copy the contents of the surface to the offscreen bitmap:
    BitBlt(OffscrDC, 0, 0, width, height, bitmapDC, 0, 0, SRCCOPY);

    // GetDIBits requires format info about the bitmap. We can have GetDIBits
    // fill a structure with that info if we pass a NULL pointer for lpvBits:
    // Reserve memory for bitmap info (BITMAPINFOHEADER + largest possible
    // palette):
    if ((lpbi = (LPBITMAPINFO)(new char[sizeof(BITMAPINFOHEADER) + 256 * sizeof(RGBQUAD)])) == NULL) 
		return false;


    ZeroMemory(&lpbi->bmiHeader, sizeof(BITMAPINFOHEADER));
    lpbi->bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
    // Get info but first de-select OffscrBmp because GetDIBits requires it:
    SelectObject(OffscrDC, OldBmp);
    if (!GetDIBits(OffscrDC, OffscrBmp, 0, height, NULL, lpbi, DIB_RGB_COLORS))
        return false;

    // Reserve memory for bitmap bits:
    if ((lpvBits = new char[lpbi->bmiHeader.biSizeImage]) == NULL)
        return false;

    // Have GetDIBits convert OffscrBmp to a DIB (device-independent bitmap):
    if (!GetDIBits(OffscrDC, OffscrBmp, 0, height, lpvBits, lpbi, DIB_RGB_COLORS))
		return false;

    // Create a file to save the DIB to:
    if ((BmpFile = CreateFile(filename,
                              GENERIC_WRITE,
                              0, NULL,
                              CREATE_ALWAYS,
                              FILE_ATTRIBUTE_NORMAL,
                              NULL)) == INVALID_HANDLE_VALUE)
							  
							  return false;

    DWORD Written;    // number of bytes written by WriteFile
    
    // Write a file header to the file:
    bmfh.bfType = 19778;        // 'BM'
    // bmfh.bfSize = ???        // we'll write that later
    bmfh.bfReserved1 = bmfh.bfReserved2 = 0;
    // bmfh.bfOffBits = ???     // we'll write that later
    if (!WriteFile(BmpFile, &bmfh, sizeof(bmfh), &Written, NULL))
        return false;

    if (Written < sizeof(bmfh)) 
		return false; 

    // Write BITMAPINFOHEADER to the file:
    if (!WriteFile(BmpFile, &lpbi->bmiHeader, sizeof(BITMAPINFOHEADER), &Written, NULL)) 
		return false;
	
    if (Written < sizeof(BITMAPINFOHEADER)) 
			return false;

    // Calculate size of palette:
    int PalEntries;
    // 16-bit or 32-bit bitmaps require bit masks:
    if (lpbi->bmiHeader.biCompression == BI_BITFIELDS) 
		PalEntries = 3;
    else
        // bitmap is palettized?
        PalEntries = (lpbi->bmiHeader.biBitCount <= 8) ?
            // 2^biBitCount palette entries max.:
            (int)(1 << lpbi->bmiHeader.biBitCount)
        // bitmap is TrueColor -> no palette:
        : 0;
    // If biClrUsed use only biClrUsed palette entries:
    if(lpbi->bmiHeader.biClrUsed) 
		PalEntries = lpbi->bmiHeader.biClrUsed;

    // Write palette to the file:
    if(PalEntries){
        if (!WriteFile(BmpFile, &lpbi->bmiColors, PalEntries * sizeof(RGBQUAD), &Written, NULL)) 
			return false;

        if (Written < PalEntries * sizeof(RGBQUAD)) 
			return false;
		}

    // The current position in the file (at the beginning of the bitmap bits)
    // will be saved to the BITMAPFILEHEADER:
    bmfh.bfOffBits = GetFilePointer(BmpFile);

    // Write bitmap bits to the file:
    if (!WriteFile(BmpFile, lpvBits, lpbi->bmiHeader.biSizeImage, &Written, NULL)) 
		return false;
	
    if (Written < lpbi->bmiHeader.biSizeImage) 
		return false;

    // The current pos. in the file is the final file size and will be saved:
    bmfh.bfSize = GetFilePointer(BmpFile);

    // We have all the info for the file header. Save the updated version:
    SetFilePointer(BmpFile, 0, 0, FILE_BEGIN);
    if (!WriteFile(BmpFile, &bmfh, sizeof(bmfh), &Written, NULL))
        return false;

    if (Written < sizeof(bmfh)) 
		return false;

    return true;
	}
	

[Disch]

Yes you have tons of memory leaks here. You're also doing some very questionable things.


With WinGDI, anything that you create (whether it's a Bitmap or DC, or whatever) must be destroyed. Furthermore, when you destroy a DC, you must revert it to its original state.

Here is an example of how you could remove the leaks from ScreenCapture:

bool ScreenCapture(int x, int y, int width, int height, char *filename){
   HDC hDc = CreateCompatibleDC(0);    // <- note, we're creating, so it needs to be destroyed
 
   HBITMAP hBmp = CreateCompatibleBitmap(GetDC(0), width, height);    // <- again, creating

   HBITMAP hBmpOld = (HBITMAP)SelectObject(hDc, hBmp);    // <- altering state
 
   BitBlt(hDc, 0, 0, width, height, GetDC(0), x, y, SRCCOPY);  // <- no significnce
 
   bool ret = SaveBMPFile(filename, hBmp, hDc, width, height);  // <- no significance




   // this is how you clean up:
   SelectObject(hDc,hBmpOld);  // put the old bitmap back in the DC (restore state)
   DeleteObject(hBmp);  // delete the bitmap we created
   DeleteDC(hDC);  // delete the DC we created
 
   return ret;
   }

You have similar problems with SaveBMPFile. What's worse, you have multiple exit points in the function at each point where an error might happen. So if an error happens, the function will exit and will not clean up properly. You can avoid this by wrapping this WinGDI stuff in some classes and using RAII techniques to have everything cleaned up in the destructors.


Another thing that really raises a red flag is here:

if ((lpbi = (LPBITMAPINFO)(new char[sizeof(BITMAPINFOHEADER) + 256 * sizeof(RGBQUAD)])) == NULL)

Why on Earth are you allocating an array of chars, only to cast it to a LPBITMAPINFO? Why not just use a BITMAPINFO struct?

Further, notice how you're new[]'ing something, but never delete[]'ing it. This is the source of another leak.

[iamthekris]

as i said, someone else wrote all this, i changed it a bit but i didnt fully understand it to the point where i knew how to fix its problems. Ill try and clean it up now, hopefully i dont miss anything

[iamthekris]

Ok so i am trying to clean up the SaveBMPFile but am having some trouble with exactly how to free up that memory, any advice?

[Disch]

The big leak is here:

if ((lpvBits = new char[lpbi->bmiHeader.biSizeImage]) == NULL)

That's allocating a bunch of space for the bitmap image, which is never freed.

Do the following:

1) change lpvBits from a LPVOID to a char*
2) make sure the function does delete[] lpvBits; before it exits (read: before every exit).

This means you'll either have to put it before every return statement, or you'll have to come up with some other way to make sure it's deleted upon exit.


Another leak:
if ((lpbi = (LPBITMAPINFO)(new char[sizeof(BITMAPINFOHEADER) + 256 * sizeof(RGBQUAD)])) == NULL)

lpbi is new[]'d, but never delete[]'d. To correct this you'll need to cast it back to a char and delete it (since it was allocated as a char*, it must be deleted as a char*):

delete[] (char*)lpbi;

Again, make sure it is deleted before the function exits.


Lastly is the WinGDI stuff. CreateCompatibleDC and CreateCompatibleBitmap need to be cleaned up with DeleteDC and DeleteObject, respectively.


EDIT:

It's also worth mentioning that new will never return a null pointer, so those if((blah = new blah) == NULL) checks are completely unnecessary.

[iamthekris]

I did what you said (thanks for taking the time to help by the way), but there still seems to be a major leak after fixing if ((lpvBits = new char[lpbi->bmiHeader.biSizeImage]) == NULL).

Perhaps i did not do it correctly, this is what i did to it.
if ((lpvBits = new char[lpbi->bmiHeader.biSizeImage]) == NULL) char *lpvBits=NULL; delete[] lpvBits;

The delete part is all the way at the end, i did not add it before every return statement, i will soon but i was thinking that since the image was being created, it was not hitting any other returns

[Disch]

if you set the pointer to null before deleting it, then you don't delete anything. That's like saying delete NULL; -- it's pointless.

if you must set it to null, do so after you delete it.

EDIT:

or am I misunderstanding what you did? Did you mean you just changed it to char*?

[iamthekris]

yah i was just changing it to char, that happened in the beginning of the function

[Disch]

The delete[] line is before the return true, right?

You'll also need to clean up the WinGDI stuff (DeleteObject, DeleteDC). That HBITMAP is probably using a pretty good chunk of memory, too.

[iamthekris]

delete[] is before the true
i understand how to delete pointers, although i might not be so sure which ones but im more confused of the WinGDI stuff, as i have never seen it before or have any idea how to get rid of it. If anyone could post a read up on it or just teach me themselves it would be nice.

[Disch]

I can't really explain it better than I did in this post:

http://cplusplus.com/forum/general/45531/#msg247181

Really you just need to delete the objects that were created, and delete the buffers that were new[]'d.

I count 5 of them:
- OffscrDC
- OffscrBmp
- BmpFile
- lpvBits
- lpbi

So to clean up you just perform the action that opposes their creation. For new[]'d buffers, this means delete[]. For WinGDI created stuff, there's other appropriate functions:

    DeleteDC(OffscrDC);    // Created with CreateCompatibleDC
    DeleteObject(OffscrBmp); // Created with CreateCompatibleBitmap
    CloseHandle(BmpFile);  // Opened with CreateFile

    delete[] lpvBits;   // allocated with new[]
    delete[] (char*)lpbi;  // allocated with new[] 

Again note that since the function exits when something fails, if you have any failure in this process the leaks will resurface. To avoid that, you'll need to either do this cleanup before every return statement or wrap everything in some kind of RTTI interface so it will automatically clean up.

[iamthekris]

This line of code seemed to help substantially

 DeleteDC(OffscrDC);    // Created with CreateCompatibleDC
    DeleteObject(OffscrBmp); // Created with CreateCompatibleBitmap
    CloseHandle(BmpFile);  // Opened with CreateFile 

Thanks a lot for that, it still has a minor leak, about 60kb per pic taken but i can try and figure that out myself. Is there something online to where i can read up on winGDI to better understand what it is doing? I did a search but all i could find were people complaining about things going wrong in their program. Im looking for more of an explenation of all of its functions and what is happening.

[Disch]

I learned from just reading the online reference material on MSDN.

http://msdn.microsoft.com/en-us/library/dd183489%28VS.85%29.aspx <- CreateCompatibleDC

You can search for other functions and structures to find out exactly what they do.

There are also some books, but I never read any. I've heard good things about Charles Petzold (sp?) books.