Stack cleanup not working (__stdcall MASM function)

Question

there's something weird going on here. Visual Studio is letting me know the ESP value was not properly saved but I cannot see any mistakes in the code (32-bit, windows, __stdcall)

MASM code:

.MODE FLAT, STDCALL
...
    memcpy PROC dest : DWORD, source : DWORD, size : DWORD
    MOV EDI, [ESP+04H]
    MOV ESI, [ESP+08H]
    MOV ECX, [ESP+0CH]
    AGAIN_:
    LODSB
    STOSB
    LOOP AGAIN_
    RETN 0CH
    memcpy ENDP

I am passing 12 bytes (0xC) to the stack then cleaning it up. I have confirmed by looking at the symbols the functions symbol goes like "memcpy@12", so its indeed finding the proper symbol

this is the C prototype:

extern void __stdcall * _memcpy(void*,void*,unsigned __int32);

Compiling in 32-bit. The function copies the memory (I can see in the debugger), but the stack cleanup appears not to be working

EDIT:

MASM code:

__MyMemcpy PROC _dest : DWORD, _source : DWORD, _size : DWORD
MOV EDI, DWORD PTR [ESP + 04H]
MOV ESI, DWORD PTR [ESP + 08H]
MOV ECX, DWORD PTR [ESP + 0CH]
PUSH ESI
PUSH EDI
__AGAIN:
LODSB
STOSB
LOOP __AGAIN
POP EDI
POP ESI
RETN 0CH
__MyMemcpy ENDP

C code:

extern void __stdcall __MyMemcpy(void*, void*, int);

typedef struct {
 void(__stdcall*MemCpy)(void*,void*,int);
}MemFunc;

int initmemfunc(MemFunc*f){
f->MemCpy=__MyMemcpy
}

when I call it like this I get the error:

MemFunc mf={0};
initmemfunc(&mf);
mf.MemCpy(dest,src,size);

when I call it like this I dont:

__MyMemcpy(dest,src,size)

Answer 1

Since you have provided an update to your question and comments suggesting you disable prologue and epilogue code generation for functions created with the MASM PROC directive I suspect your code looks something like this:

.MODEL FLAT, STDCALL
OPTION PROLOGUE:NONE
OPTION EPILOGUE:NONE

.CODE

__MyMemcpy PROC _dest : DWORD, _source : DWORD, _size : DWORD
    MOV EDI, DWORD PTR [ESP + 04H]
    MOV ESI, DWORD PTR [ESP + 08H]
    MOV ECX, DWORD PTR [ESP + 0CH]
    PUSH ESI
    PUSH EDI
__AGAIN:
    LODSB
    STOSB
    LOOP __AGAIN
    POP EDI
    POP ESI
    RETN 0CH
__MyMemcpy ENDP

END

A note about this code: beware that if your source and destination buffers overlap this can cause problems. If the buffers don't overlap then what you are doing should work. You can avoid this by marking the pointers __restrict. __restrict is an MSVC/C++ extension that will act as a hint to the compiler that the argument doesn't overlap with another. This can allow the compiler to potentially warn of this situation since your assembly code is unsafe for that situation. Your prototypes could have been written as:

extern void __stdcall __MyMemcpy( void* __restrict, void* __restrict, int);

typedef struct {
    void(__stdcall* MemCpy)(void* __restrict, void* __restrict, int);
}MemFunc;

You are using PROC but not taking advantage of any of the underlying power it affords (or obscures). You have disabled PROLOGUE and EPILOGUE generation with the OPTION directive. You properly use RET 0Ch to have the 12 bytes of arguments cleaned from the stack.

From a perspective of the STDCALL calling convention your code is correct as it pertains to stack usage. There is a serious issue in that the Microsoft Windows STDCALL calling convention requires the caller to preserve all the registers it uses except EAX, ECX, and EDX. You clobber EDI and ESI and both need to be saved before you use them. In your code you save them after their contents are destroyed. You have to push both ESI and EDI on the stack first. This will require you adding 8 to the offsets relative to ESP. Your code should have looked like this:

__MyMemcpy PROC _dest : DWORD, _source : DWORD, _size : DWORD
    PUSH EDI                         ; Save registers first
    PUSH ESI
    MOV EDI, DWORD PTR [ESP + 0CH]   ; Arguments are offset by an additional 8 bytes
    MOV ESI, DWORD PTR [ESP + 10H]
    MOV ECX, DWORD PTR [ESP + 14H]
__AGAIN:
    LODSB
    STOSB
    LOOP __AGAIN
    POP ESI                          ; Restore the caller (non-volatile) registers
    POP EDI
    RETN 0CH
__MyMemcpy ENDP

You asked the question why it appears you are getting an error about ESP or a stack issue. I assume you are getting an error similar to this:

This could be a result of either ESP being incorrect when mixing STDCALL and CDECL calling conventions or it can arise out of the value of the saved ESP being clobbered by the function. It appears in your case it is the latter.

I wrote a small C++ project with this code that has similar behaviour to your C program:

#include <iostream>

extern "C" void __stdcall __MyMemcpy( void* __restrict, void* __restrict, int);

typedef struct {
    void(__stdcall* MemCpy)(void* __restrict, void* __restrict, int);
}MemFunc;

int initmemfunc(MemFunc* f) {
    f->MemCpy = __MyMemcpy;
    return 0;
}

char buf1[] = "Testing";
char buf2[200];

int main()
{
    MemFunc mf = { 0 };
    initmemfunc(&mf);
    mf.MemCpy(buf2, buf1, strlen(buf1));
    std::cout << "Hello World!\n" << buf2;
}

When I use code like yours that doesn't properly save ESI and EDI I discovered this in the generated assembly code displayed in the Visual Studio C/C++ debugger:

I have annotated the important parts. The compiler has generated C runtime checks (these can be disabled, but they will just hide the problem and not fix it) including a check of ESP across a STDCALL function call. Unfortunately it relies on saving the original value of ESP (before pushing parameters) into the register ESI. As a result a runtime check is made after the call to __MyMemcpy to see if ESP and ESI are still the same value. If they aren't you get the warning about ESP not being saved correctly.

Since your code incorrectly clobbers ESI (and EDI) the check fails. I have annotated the debug output to hopefully provide a better explanation.

---

You can avoid the use of a LODSB/STOSB loop to copy data. There is an instruction that just this very operation (REP MOVSB) that copies ECX bytes pointed to by ESI and copies them to EDI. A version of your code could have been written as:

__MyMemcpy PROC _dest : DWORD, _source : DWORD, _size : DWORD
    PUSH EDI                         ; Save registers first
    PUSH ESI
    MOV EDI, DWORD PTR [ESP + 0CH]   ; Arguments are offset by an additional 8 bytes
    MOV ESI, DWORD PTR [ESP + 10H]
    MOV ECX, DWORD PTR [ESP + 14H]
    REP MOVSB
    POP ESI                          ; Restore the caller (non-volatile) registers
    POP EDI
    RETN 0CH
__MyMemcpy ENDP

---

If you were to use the power of PROC to save the registers ESI and EDI you could list them with the USES directive. You can also reference the argument locations on the stack by name. You can also have MASM generate the proper EPILOGUE sequence for the calling convention by simply using ret. This will clean the up the stack appropriately and in the case of STDCALL return by removing the specified number of bytes from the stack (ie ret 0ch) in this case since there are 3 4-byte arguments.

The downside is that you do have to generate the PROLOGUE and EPILOGUE code that can make things more inefficient:

.MODEL FLAT, STDCALL
.CODE

__MyMemcpy  PROC USES ESI EDI dest : DWORD, source : DWORD, size : DWORD
    MOV EDI, dest
    MOV ESI, source
    MOV ECX, size
    REP MOVSB                        ; Use instead of LODSB/STOSB+Loop  
    RET
__MyMemcpy  ENDP

END

The assembler would generate this code for you:

PUBLIC __MyMemcpy@12
__MyMemcpy@12:
    push        ebp  
    mov         ebp,esp            ; Function prologue generate by PROC
    push        esi                ; USES caused assembler to push EDI/ESI on stack
    push        edi  
    mov         edi,dword ptr [ebp+8]  
    mov         esi,dword ptr [ebp+0Ch]  
    mov         ecx,dword ptr [ebp+10h]  
    rep movs    byte ptr es:[edi],byte ptr [esi]
    ; MASM generated this from the simple RET instruction to restore registers,
    ; clean up stack and return back to caller per the STDCALL calling convention
    pop         edi                ; Assembler
    pop         esi  
    leave  
    ret         0Ch  

Some may rightly argue that having the assembler obscure all this work makes the code potentially harder to understand for someone who doesn't realize the special processing MASM can do with a PROC declared function. This may result in harder to maintain code for someone else that is unfamiliar with MASM's nuances in the future. If you don't understand what MASM may generate, then sticking to coding the body of the function yourself is probably a safer bet. As you have found that also involves turning PROLOGUE and EPILOGUE code generation off.

Answer 2

The reason why the stack is corrupted is that MASM "secretly" inserts the prologue code to your function. When I added the option to disable that, the function works for me now.

You can see this, when you switch to assembly mode while still in the C code and then step into your function. It seems that VS doesn't swtich to assembly mode when already in the assembly source.

.586
.MODEL FLAT,STDCALL
OPTION PROLOGUE:NONE 

.CODE

mymemcpy PROC dest:DWORD, src:DWORD, sz:DWORD
    MOV EDI, [ESP+04H]
    MOV ESI, [ESP+08H]
    MOV ECX, [ESP+0CH]
AGAIN_:
    LODSB
    STOSB
    LOOP AGAIN_
    RETN 0CH
mymemcpy ENDP

END