Unknown memory leak

[Stifado]

...my very second program/exercise is experiencing a memory leak, every time it spawns a MessageBox.

The code that projects this MessageBox is within a thread procedure, because I want to process & display multiple messages in a multithreaded enviroment:

Code Select Expand



WinsockGetData proc

local SenderIP: dword
local Sender: sockaddr_in
local SenderLen: dword


invoke WaitForSingleObject, hInBufferMutex, INFINITE

    mov SenderLen, sizeof sockaddr_in
    invoke recvfrom, hSocket, addr InBuffer, sizeof InBuffer, 0, addr Sender, addr SenderLen

    invoke ReleaseMutex, hInBufferMutex

    push Sender.sin_addr
    call inet_ntoa
    mov SenderIP, eax

    invoke MessageBox, 0, addr InBuffer, SenderIP, MB_ICONINFORMATION

invoke ExitThread, 0

WinsockGetData endp



I'm not sure but I think this leak is caused by event objects that do not close after the MessageBox is closed (or maybe this is one of the many different leaks :P). Everytime a MessageBox is created, it creates 2 event objects (I don't know why :P) Some of them, while in a burst, do not close.

• Is there a way to track 'em down and close?

[evlncrn8]

well if you're using releasemutex with the idea that it free's it.. you're wrong
you need to CloseHandle on the mutex to close it
sounds like your events are piling up.....

can you explain what you're trying to do?

[Vortex]

The add esp,4 statement, is it associated with push Sender.sin_addr or something else ?

[Stifado]

Don't worry about ReleaseMutex, I have read MSDN. This mutex handle is global and it is invalidated when the main program ends.
I also use CloseHandle with the newly created thread. Well, as a general rule, I alaways close handles that are not of use anymore.

The problem arises from MessageBox, I think...

The program is a UDP chat, that displays the messages with message boxes. Say, something similar to the Messenger service, but it does not use mailslots...

As for "add esp, 4", yeah you're right. It's useless. I thought I needed to pop the argument that passed to the procedure. But it's wrong. ExitThread do this, right? I'll correct it to avoid confusion.

[Vortex]

Hi Stifado,

You should terminate the procedure with a ret instruction instead of ExitThread. Masm will balance the stack to free the parameter(s) passed to the procedure. Note that the stdcall calling convention should be selected at the top of the source code to enable this feature. Note that the C calling convention handles the stack balancing differently.

Let's assume you pass two parameter to your procedure :

Code Select Expand

abc PROC x:DWORD,y:DWORD
.
.
ret
abc ENDP

Masm will put a ret 8 instruction at the epilogue code to terminate the procedure.

[Stifado]

I have read the "Art of Assembly", so I am considered to know much of the basics. Yes, I know about the calling conventions, and yes, I use the stdcall calling convention because I've read that Win API uses this one...

Using a simple "ret" instead of ExitThread... I have tried this, but doesn't change anything!

Okay, I'll make use of a "ret" instruction instead of ExitThread...

But I don't think calling ExitThread will mess up things because:

"... When this function is called (either explicitly or by returning from a thread procedure), the current thread's stack is deallocated, all pending I/O initiated by the thread is cancelled, and the thread terminates. ..." from MSDN

... ???

[Vortex]

Here is a simple demonstration :

Code Select Expand


.386
.model flat,stdcall
option casemap : none

include     \masm32\include\windows.inc
include     \masm32\include\kernel32.inc
include     \masm32\include\user32.inc

includelib  \masm32\lib\kernel32.lib
includelib  \masm32\lib\user32.lib
test1 PROTO :DWORD,:DWORD

.data
format1     db 'x + y = %d',0
capt        db 'Procedure demo',0

.data?
buffer  db 100 dup(?)

.code

start:

    invoke  test1,75,25
    invoke  wsprintf,ADDR buffer,ADDR format1,eax
    invoke  MessageBox,0,ADDR buffer,ADDR capt,MB_OK
    invoke  ExitProcess,0

test1 PROC x:DWORD,y:DWORD

    mov     eax,x
    add     eax,y
    invoke  ExitThread,0

test1 ENDP

END start

Here, ExitThread will terminate the application, so there will be no access to the continuation of the code, invoke wsprintf etc. Replacing ExitThread with ret, the procedure will return the control the main module.

I have read the "Art of Assembly", so I am considered to know much of the basics. Yes, I know about the calling conventions, and yes, I use the stdcall calling convention because I've read that Win API uses this one...

I am trying to be good intended and I am just trying to help you...

edit : sorry, I missed you are talking about multithreading, you can omit the code above.

[Stifado]

Hey! I just stated what I have read, just to tell you what I am aware of, so you won't waste your time explaining to me. That 'll speed up things :P For God's sake, no offense meant, and for God's sake, noone can just know everything! Not even me :P

[evlncrn8]

tried debugging your code then?

[Stifado]

Well, that's what I'm trying to do... find the bug or in this case the leak...

I'll attach the source code, in case someone would like to help with it in first hand.

[attachment deleted by admin]

[Stifado]

I'll quit searching this deadend. If I ever find it, I'll report it :P

[six_L]

Code Select Expand

.686
.model flat,stdcall
option casemap:none
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
include \masm32\include\windows.inc
include \masm32\include\kernel32.inc
include \masm32\include\user32.inc
include \masm32\include\gdi32.inc
include \masm32\include\ole32.inc
include \masm32\include\oleaut32.inc
include \masm32\include\Comctl32.inc
include \masm32\include\masm32.inc
include \masm32\include\wsock32.inc

includelib \masm32\lib\kernel32.lib
includelib \masm32\lib\user32.lib
includelib \masm32\lib\gdi32.lib
includelib \masm32\lib\ole32.lib
includelib \masm32\lib\oleaut32.lib
includelib \masm32\lib\Comctl32.lib
includelib \masm32\lib\masm32.lib
includelib \masm32\Lib\wsock32.lib
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
include \masm32\macros\macros.asm
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
frmMain equ 101
FormIcon equ 102
txtMessage equ 1002
txtRecipient equ 1003
cmdSend equ 1004
cmdExit equ 1005
lblStatus equ 1006
lblIPAddress equ 1007

SMM_PORT equ 1a02h
OUT_BUFFER_SIZE equ 1024
IN_BUFFER_SIZE equ 1024
WM_SOCKET       equ WM_USER + 100
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
.data?
Buffer db OUT_BUFFER_SIZE dup (?)
InBuffer db IN_BUFFER_SIZE dup (?)
hSocket SOCKET ?
hInstance HINSTANCE ?
hIcon HICON ?
hDialog HWND ?
hInBufferMutex HANDLE ?
x WSADATA<?>
hStopEvent dd ?
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
.code
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
SetStatus proc text: dword

invoke SetDlgItemText, hDialog, lblStatus, text
ret

SetStatus endp
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
WinsockSendData proc
local Recipient: sockaddr_in
local MsgSize: dword

invoke  RtlZeroMemory,addr Buffer,sizeof Buffer
invoke GetDlgItemText, hDialog, txtRecipient, addr Buffer, sizeof Buffer
        .if eax != 0
invoke inet_addr, addr Buffer
                .if eax != INADDR_NONE
mov Recipient.sin_addr, eax
  invoke htons,SMM_PORT
mov Recipient.sin_port, ax
                        mov Recipient.sin_family, AF_INET

invoke  RtlZeroMemory,addr Buffer,sizeof Buffer
invoke GetDlgItemText, hDialog, txtMessage, addr Buffer, sizeof Buffer
                        .if eax != 0
mov MsgSize, eax
inc MsgSize
invoke sendto, hSocket, addr Buffer, MsgSize, 0, addr Recipient, sizeof sockaddr_in
.if eax == SOCKET_ERROR
invoke WSAGetLastError
.else
xor eax, eax ; Message sent
.endif
                        .else
mov eax, 3 ; Message is empty
                        .endif
                .else
mov eax, 2 ; Invalid IPv4 address
                .endif
.else
mov eax, 1 ; No IPv4 address specified
        .endif
ret

WinsockSendData endp
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
; WARNING - This is a thread procedure!
WinsockGetData proc lpParameter: LPVOID
local SenderIP: dword
local Sender: sockaddr_in
local SenderLen: dword


invoke WaitForSingleObject,hStopEvent,INFINITE

invoke  RtlZeroMemory,addr InBuffer,sizeof InBuffer

        mov SenderLen, sizeof sockaddr_in
        invoke recvfrom, hSocket, addr InBuffer, sizeof InBuffer, 0, addr Sender, addr SenderLen

push Sender.sin_addr
call inet_ntoa
add esp,4

mov SenderIP, eax

        invoke MessageBox, 0, addr InBuffer, SenderIP, MB_ICONINFORMATION

invoke ResetEvent,hStopEvent
invoke closesocket, hSocket
jmp WinsockGetData
ret

WinsockGetData endp
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
GetLocalIP proc

invoke gethostbyname,0

lea eax, [eax+12]
mov eax, [eax]
mov eax, [eax]
mov eax, [eax]

ret

GetLocalIP endp
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
DialogProc proc hwndDlg: HWND, uMsg: UINT, wParam: WPARAM, lParam: LPARAM
local y: sockaddr_in

        mov eax,uMsg
.if eax == WM_INITDIALOG

push hwndDlg
pop hDialog

invoke SendMessage, hwndDlg, WM_SETICON, ICON_SMALL, hIcon
invoke SendMessage, hwndDlg, WM_SETICON, ICON_BIG, hIcon

invoke GetLocalIP
invoke inet_ntoa, eax
invoke SetDlgItemText, hwndDlg, lblIPAddress , eax

invoke CreateEvent,NULL,TRUE,FALSE,NULL
mov hStopEvent,eax

invoke CreateThread, 0, 0, addr WinsockGetData, 0, 0, 0
invoke CloseHandle, eax

.elseif eax == WM_CLOSE
QuitProgram:
invoke EndDialog, hwndDlg, 0
.elseif eax == WM_COMMAND
                .if word ptr wParam == cmdSend

invoke socket, AF_INET, SOCK_DGRAM, IPPROTO_UDP
.if eax != INVALID_SOCKET
mov hSocket, eax

invoke  RtlZeroMemory,addr y,sizeof y
mov y.sin_family, AF_INET
  invoke htons,SMM_PORT
mov y.sin_port, ax
mov y.sin_addr, 0
invoke bind, hSocket, addr y, sizeof sockaddr_in
invoke WSAAsyncSelect, hSocket, hwndDlg, WM_SOCKET, FD_READ

invoke  WinsockSendData
push eax
.if eax == 0
fn SetStatus, "Message sent"
.elseif eax == 1
fn SetStatus, "No IPv4 address specified"
.elseif eax == 2
fn SetStatus, "Invalid IPv4 address"
.elseif eax == 3
fn SetStatus, "The message is empty"
.else
.if eax == WSAEHOSTUNREACH
fn SetStatus, "Destination unreachable"
.elseif eax == WSAEADDRNOTAVAIL
fn SetStatus, "Address not available"
.else
fn SetStatus, "Unknown error"
.endif
.endif

pop eax
.if eax!=0
invoke closesocket, hSocket
invoke  MessageBeep,MB_ICONHAND
.endif
.else
fn MessageBox, 0, "Failed to initialize Winsock!", "SMM - Error", MB_ICONERROR
jmp QuitProgram
.endif

                .elseif word ptr wParam == cmdExit
jmp QuitProgram
                .endif

.elseif eax == WM_SOCKET
invoke SetEvent,hStopEvent
.else
mov eax,FALSE
ret
.endif
mov eax,TRUE
ret

DialogProc endp
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
start:
invoke GetModuleHandle, 0
mov hInstance, eax

invoke LoadIcon, hInstance, FormIcon
mov hIcon, eax

invoke WSAStartup, 0202h, addr x
.if eax==0

invoke DialogBoxParam, hInstance, frmMain, 0, addr DialogProc, 0
invoke WSACleanup

.else
fn MessageBox, 0, "Failed to initialize Winsock!", "SMM - Error", MB_ICONERROR
.endif

exit
;>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

end start


[Stifado]

Thanks for posting, six_L. Let's see...

SMM_PORT equ 1a02h

Is there something wrong with 9a02h that I should be aware of? :P Unless you play Doom, you can use it. :)

WM_SOCKET equ WM_USER + 100

I've searched MSDN about WM_USER. That suits better than RegisterWindowMessage. Thanks.

...

invoke RtlZeroMemory,addr Buffer,sizeof Buffer
invoke GetDlgItemText, hDialog, txtRecipient, addr Buffer, sizeof Buffer

...

Well, in my opinion, using RtlZeroMemory is useless (just a waste of time), because GetDlgItemText reads and the NULL char.

invoke htons,SMM_PORT

I have already byte swapped the port that's why I put it on a compiler constant... Is that wrong?

...

invoke RtlZeroMemory,addr Buffer,sizeof Buffer
invoke GetDlgItemText, hDialog, txtMessage, addr Buffer, sizeof Buffer

...

Later in sendto I specify the bytes to send... Why use RtlZeroMemory? I mean, there's nothing bad about overlapping data...

invoke WaitForSingleObject,hStopEvent,INFINITE

Using an event object to synchronize the thread that reads the data from the socket. Ok by me.

...

invoke  RtlZeroMemory,addr InBuffer,sizeof InBuffer

mov SenderLen, sizeof sockaddr_in
invoke recvfrom, hSocket, addr InBuffer, sizeof InBuffer, 0, addr Sender, addr SenderLen

...

Yet another one, (useless) memory clear operation. Ok, to say the truth, if you must pass a structure with a lot of fields to some function, it would be a nice idea to clear it whole first, and then assign the fields you want accordingly. So, the fields that you were to set to zero, will be already zero. But, this is not the case. Actually, recvfrom writes data to the buffer and returns how much bytes has read. Which means that you know what data to discard. Anyway, later the MessageBox function waits for a null terminating string. By the time the data includes this NULL char, there should be no problem at all.

...

push Sender.sin_addr
call inet_ntoa
add esp,4

...

I can't understand "add esp, 4"... help please.

...

invoke ResetEvent,hStopEvent
invoke closesocket, hSocket
jmp WinsockGetData

ret

...

"jmp WinsockGetData": I guessed that before debugging it: This one will rebuild the stack frame, again and again... well until the stack overflows... that'll need some time though, so it's acceptable :P

...

invoke bind, hSocket, addr y, sizeof sockaddr_in
invoke WSAAsyncSelect, hSocket, hwndDlg, WM_SOCKET, FD_READ

    invoke  WinsockSendData
    push eax
   .if eax == 0
   Â    fn SetStatus, "Message sent"
   .elseif eax == 1
      fn SetStatus, "No IPv4 address specified"
   .elseif eax == 2
      fn SetStatus, "Invalid IPv4 address"
   .elseif eax == 3
      fn SetStatus, "The message is empty"
   .else
      .if eax == WSAEHOSTUNREACH
         fn SetStatus, "Destination unreachable"
      .elseif eax == WSAEADDRNOTAVAIL
         fn SetStatus, "Address not available"
      .else
         fn SetStatus, "Unknown error"
      .endif
   .endif
            
   pop eax
   .if eax!=0
      invoke   closesocket, hSocket
      invoke  MessageBeep,MB_ICONHAND
   .endif

...

First of all, you invoke bind only when the user decides to send something which means that he won't be able to receive data, right?
Also, I can't figure exactly what goes wrong, but If the user sends a message to an address that is invalid, and then sends a message to a valid one, this code will keep creating sockets, thus generating a leak.

After "invoke WinsockSendData" you push eax, because SetStatus messes up eax value which you want to check after this "if" statement. I have corrected this one in my code. By the time SetStatus returns nothing it would be nice to preserve eax. Thanks again.


Can't really say that your program works, but I learned some things from it. Thanks again :)

[six_L]

hey,Stifado 

before you use a buffer or Structure, Clearing its maybe a good habit.
if my code can't work, normally i first use some stupid way to make it running, no matter how it's mess.

add esp,4

for balancing the stack.

the trouble of your code isn't the memory leak,is the hSocket's trouble. Service's hSocket isn't same client's hSocket.

can you explain what you're trying to do?

[server_1]~~Message
       |<--------- [client_2]

[client_1]----->[server_2]~~Message

the responsibility for recv msg and show msg belong to server's thread (main thread)
the responsibility for send msg belong to client's thread(child thread)
is it right?

[Stifado]

if my code can't work, normally i first use some stupid way to make it running, no matter how it's mess.

And what does this supposed to mean? I've never heard of this "stupid" way to develop logic programs. I don't think it's possible to first write the code and then think of it... that sounds stupid, indeed :P

add esp,4
for balancing the stack

Balancing what? Ehm, so what's the value that should be popped of the stack? Where it came from? I mean...

Code Select Expand


WinsockGetData proc lpParameter: LPVOID
local SenderIP: dword
local Sender: sockaddr_in
local SenderLen: dword


invoke WaitForSingleObject,hStopEvent,INFINITE

invoke RtlZeroMemory,addr InBuffer,sizeof InBuffer

        mov SenderLen, sizeof sockaddr_in
        invoke recvfrom, hSocket, addr InBuffer, sizeof InBuffer, 0, addr Sender, addr SenderLen

push Sender.sin_addr
call inet_ntoa
add esp,4

mov SenderIP, eax

        invoke MessageBox, 0, addr InBuffer, SenderIP, MB_ICONINFORMATION

invoke ResetEvent,hStopEvent
invoke closesocket, hSocket
jmp WinsockGetData
ret

WinsockGetData endp


• push ebp
  mov ebp, esp
[/b]

    (or substitute this with ENTER) This adds a dword on the stack.



[li]local SenderIP: dword
   local Sender: sockaddr_in
   local SenderLen: dword[/li]

    This one leaves 24B on the stack.


[li]invoke WaitForSingleObject,hStopEvent,INFINITE[/li]

    This one doesn't leave garbage on the stack.


[li]invoke  RtlZeroMemory,addr InBuffer,sizeof InBuffer[/li]

    I can't see anything left on the stack while debuging...


[li]invoke recvfrom, hSocket, addr InBuffer, sizeof InBuffer, 0, addr Sender, addr SenderLen[/li]

    Nope.


[li]   push   Sender.sin_addr
   call   inet_ntoa[/li]

    Nah.


[li]invoke MessageBox, 0, addr InBuffer, SenderIP, MB_ICONINFORMATION[/li]

    It's an API call after all. It uses stdcall. Or not?

[/list]

Maybe it's something that I'm not aware of...? Please, explain. Need to know.

the trouble of your code isn't the memory leak,is the hSocket's trouble. Service's hSocket isn't same client's hSocket.

Well, I don't have any experience in Winsock *API* programming... what do you mean "Service's hSocket isn't same client's hSocket."?

can you explain what you're trying to do?

Merely a simple UDP messagebox-driven chat program. Basically to exercise on API calls.

the responsibility for recv msg and show msg belong to server's thread (main thread)

Am I writting incomprehensible code? :( Everytime the socket is ready to read data, it spawns a thread that will read the data and display the MessageBox.

the responsibility for send msg belong to client's thread(child thread)

Actually, the main thread is the one that sends the data...

is it right?

Definately, no :P