2016-07-09 10 views
-4

私はこのスクリーンキャプチャコードのどこかにかなり大きなリークがあります。誰もが素晴らしいだろうな問題を指摘することができますので、もしC++メモリリーク - ピアレビューが必要

私のC++はかなり悪いです!

ここで問題のあるコードです:

FString AWindow::CaptureWindow(HWND hwnd) { 

HDC hdcSrc = GetWindowDC(hwnd); 
RECT rawRect; 
LPRECT rect = &rawRect; 

GetWindowRect(hwnd, rect); 

int width = rect->right - rect->left; 
int height = rect->bottom - rect->top; 

HDC hdcDest = CreateCompatibleDC(hdcSrc); 
HBITMAP hBitmap = CreateCompatibleBitmap(hdcSrc, width, height); 

HGDIOBJ h0ld = SelectObject(hdcDest, hBitmap); 
BitBlt(hdcDest, 0, 0, width, height, hdcSrc, 0, 0, SRCCOPY); 
SelectObject(hdcDest, h0ld); 
DeleteDC(hdcDest); 

    char* pImage = NULL; 
    pImage = (char*)GlobalLock(hBitmap); 


BITMAP bmp; 
PBITMAPINFO pbmi; 
WORD cClrBits; 

GetObject(hBitmap, sizeof(BITMAP), (LPSTR)&bmp); 

//Convert the color format to a count of bits 
cClrBits = (WORD)(bmp.bmPlanes * bmp.bmBitsPixel); 

if (cClrBits == 1) 
    cClrBits = 1; 
else if (cClrBits <= 4) 
    cClrBits = 4; 
else if (cClrBits <= 8) 
    cClrBits = 8; 
else if (cClrBits <= 16) 
    cClrBits = 16; 
else if (cClrBits <= 24) 
    cClrBits = 24; 
else cClrBits = 32; 

//Allocate memory for the BITMAPINFO structure. 
if (cClrBits < 24) { 
    pbmi = (PBITMAPINFO)LocalAlloc(LPTR, sizeof(BITMAPINFOHEADER) + sizeof(RGBQUAD) * (1i64 << cClrBits)); 
} 
else { 
    pbmi = (PBITMAPINFO)LocalAlloc(LPTR, sizeof(BITMAPINFOHEADER)); 
} 

//Initialize the field in the BITMAPINFO structure 
pbmi->bmiHeader.biWidth = bmp.bmWidth; 
pbmi->bmiHeader.biHeight = bmp.bmHeight; 
pbmi->bmiHeader.biPlanes = bmp.bmPlanes; 
pbmi->bmiHeader.biBitCount = bmp.bmBitsPixel; 
pbmi->bmiHeader.biSize = sizeof(BITMAPINFOHEADER); 
pbmi->bmiHeader.biClrUsed = (1i64 << cClrBits); 

pbmi->bmiHeader.biCompression = BI_RGB; 

//Computer the number of bytes in the array of color 
//indices and store the result in biSizeImage. 
pbmi->bmiHeader.biSizeImage = ((pbmi->bmiHeader.biWidth * cClrBits + 31) & ~31)/8 
    * pbmi->bmiHeader.biHeight; 

//Set biClrImportant to 0, indicating that all of the device 
//colors are important 
pbmi->bmiHeader.biClrImportant = 0; 

FString file = FPaths::Combine(*FPaths::GameDir(), TEXT("ScreenGrab/"), TEXT("Desktop.bmp")); 

std::string mystring(TCHAR_TO_UTF8(*file)); 
std::wstring lpstring = std::wstring(mystring.begin(), mystring.end()); 
LPCWSTR realfile = lpstring.c_str(); 

FString error = CreateBMPFile(hwnd, realfile, pbmi, hBitmap, hdcSrc); 

ReleaseDC(hwnd, hdcSrc); 
DeleteObject(hBitmap); 
return error; 

}

は私が、私は、彼らがしているものは考えているものの、削除し忘れてるそこにいくつかのオブジェクトがあります推測しています。私は問題のあるものを特定することができるかどうかを確認する時に行をコードをコメントアウトしながら、

それをここに投稿。

おかげ -Paul

:: EDIT ::

さらに検査時に私は、BMPを保存する機能まで深刻なリークを縮小しました。

FString AWindow::CreateBMPFile(HWND hwnd, LPCWSTR pszFile, PBITMAPINFO pbi, HBITMAP hBMP, HDC hDC) { 
HANDLE hf;     // file handle 
BITMAPFILEHEADER hdr;  // bitmap file-header 
PBITMAPINFOHEADER pbih;  // bitmap info-header 
LPBYTE lpBits;    // memory pointer 
DWORD dwTotal;    // total count of bytes 
DWORD cb;     // incremental count of bytes 
BYTE *hp;     // byte pointer 
DWORD dwTmp; 

pbih = (PBITMAPINFOHEADER)pbi; 
lpBits = (LPBYTE)GlobalAlloc(GMEM_FIXED, pbih->biSizeImage); 

if (!lpBits) { 
    return FString("global alloc failed"); 
} 


// Retrieve the color table (RGBQUAD array) and the bits 
// (array of palette indices) from the DIB. 
if (!GetDIBits(hDC, hBMP, 0, (WORD)pbih->biHeight, lpBits, pbi, 
    DIB_RGB_COLORS)) 
{ 
    return FString("get di bits failed"); 
} 

// Create the .BMP file. 
hf = CreateFile(pszFile, 
    GENERIC_READ | GENERIC_WRITE, 
    (DWORD)0, 
    NULL, 
    CREATE_ALWAYS, 
    FILE_ATTRIBUTE_NORMAL, 
    (HANDLE)NULL); 
if (hf == INVALID_HANDLE_VALUE) 
{ 
    return FString("invalid file handle"); 
} 
hdr.bfType = 0x4d42;  // 0x42 = "B" 0x4d = "M" 
          // Compute the size of the entire file. 
hdr.bfSize = (DWORD)(sizeof(BITMAPFILEHEADER) + 
    pbih->biSize + pbih->biClrUsed 
    * sizeof(RGBQUAD) + pbih->biSizeImage); 
hdr.bfReserved1 = 0; 
hdr.bfReserved2 = 0; 

// Compute the offset to the array of color indices. 
hdr.bfOffBits = (DWORD) sizeof(BITMAPFILEHEADER) + 
    pbih->biSize + pbih->biClrUsed 
    * sizeof(RGBQUAD); 

// Copy the BITMAPFILEHEADER into the .BMP file. 
if (!WriteFile(hf, (LPVOID)&hdr, sizeof(BITMAPFILEHEADER), 
    (LPDWORD)&dwTmp, NULL)) 
{ 
    return FString("Write header failed"); 
} 

// Copy the BITMAPINFOHEADER and RGBQUAD array into the file. 
if (!WriteFile(hf, (LPVOID)pbih, sizeof(BITMAPINFOHEADER) 
    + pbih->biClrUsed * sizeof(RGBQUAD), 
    (LPDWORD)&dwTmp, (NULL))) { 
    return FString("write info header failed"); 
} 


// Copy the array of color indices into the .BMP file. 
dwTotal = cb = pbih->biSizeImage; 
hp = lpBits; 
if (!WriteFile(hf, (LPSTR)hp, (int)cb, (LPDWORD)&dwTmp, NULL)) { 
    return FString("copy color failed"); 
} 

// Close the .BMP file. 
if (!CloseHandle(hf)) 


    // Free memory. 
    GlobalFree((HGLOBAL)lpBits); 

return FString("Completed ok?"); 

}

おかげで再びすべてのヘルプは:です。ここ

!この混乱を解決する誰のためのデジタルブラウニー!

+1

リソースを解放して閉じるためにスマートポインタ(カスタムの削除者を使用)を使用することを検討してください。これにより、どこからでも手動で管理する必要がなくなります。 –

+0

Btw、この質問はhttp://codereview.stackexchange.comの方が適しています。 –

+1

ああ、codereviewが事だったのを知りませんでした。なぜ私は下の票をすべて持っているのだろうと思っていた。 私は、スマートポインタは、Unrealのエンジンであるか互換性がありません確信しているが、私は前方に移動すると:) – Ozzadar

答えて

4

"pbmi"がローカルに割り当てられました。

あなたは割り当てを解除し、「LocalFree」を使うのを忘れていました。

は多分もっとありますが、それは私が見つけた最初のものでした。 RAII使用

+1

ありがとう!変更を加えました! 実際には、重大なリークをCreateBMPFileにさらに絞り込みました。これは、投稿の一番下の編集に追加しています。 マインドを見ますか? (この関数は連続して大量に呼び出され、すぐにメモリMBを獲得します) – Ozzadar

+0

返す前にGlobalFreeする必要があります。メモリを大量に節約します。あなたが二度自由にならないようにしてください。 –

+0

ああ、もう1つは、すべての戻りの前にCloseHandleを使用します。ハンドルは開いたままになり、リークが発生します。 –

0

あなたが閉じを振りかけるする必要はありません、パターン(リソース集録は、初期化され)、各リターンの前に解放されます。さらに、例外がスローされた場合(たとえば、UTF8変換での不正な文字エンコードなど)、リークを防ぐことができます。ここで@JesperJuhlはあなたのコードのコンテキストで参照しているイディオムは次のとおりです。

using file_raii_t = std::unique_ptr<std::remove_pointer<HANDLE>::type, decltype(&::CloseHandle)>; 
using gmem_raii_t = std::unique_ptr<std::remove_pointer<HGLOBAL>::type, decltype(&::GlobalFree)>; 

gmem_raii_t gmem(::GlobalAlloc(GMEM_FIXED, size), ::GlobalFree); 
// gmem can be used like (!GetDIBits(..., gmem.get(), ...); 

file_raii_t fh(::CreateFile(pszFile, GENERIC_READ | GENERIC_WRITE, (DWORD)0, NULL, 
    CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, (HANDLE)NULL), ::CloseHandle); 
//fh can be used like FileRead(fh.get()...); 

これは、他のAPI(outパラメータなどのリソースを返すも、API)を含むように拡張することができます。あなたの場合:GetWindowDC、LocalAlloc。 RAIIを使用すると、より少ない行の例外セーフコードを書くことができます。

注:私のエージングメモリのコード。あなたのコードではテストされていません。