Simple encryption Assembly Program - Access violation writing to memory location

I had to implement a cdecl calling convention into this program which originally used a non standardized convention. As far as I can tell it looks right, but I get a unhandled exception error saying "Accress violation writing location 0x00000066, which seems to hit when the program gets down to the line "not byte ptr[eax]" or atleast that is where the arrow points after breaking the program.

Could anyone tell me what is wrong with my program and how I may fix it? Thank you.

void encrypt_chars (int length, char EKey)
{   char temp_char;                     

for (int i = 0; i < length; i++)    
{
    temp_char = OChars [i];         
    __asm {
            push   eax
            movzx  eax, temp_char
            push   eax
            lea    eax, EKey
            push   eax
            call   encrypt
            mov    temp_char, al

            pop    eax
    }
    EChars[i] = temp_char;          
return;


// Inputs: register EAX = 32-bit address of Ekey,
//                  ECX = the character to be encrypted (in the low 8-bit field, CL).
// Output: register EAX = the encrypted value of the source character (in the low 8-bit field, AL).

__asm {          

encrypt:

        push ebp
        mov ebp, esp
        mov ecx, 8[ebp] 
        mov eax, 12[ebp]
        push edi                  
        push ecx                  
        not byte ptr[eax]         
        add byte ptr[eax], 0x04   
        movzx edi, byte ptr[eax]  
        pop eax                   
        xor eax, edi              
        pop edi                   
        rol al, 1                 
        rol al, 1                 
        add al, 0x04              
        mov esp, ebp
        pop ebp
        ret                       
}

By inspection, the comment on the encrypt function is wrong. Remember: the stack grows down, so when the arguments are pushed onto the stack, the ones pushed first have the higher address and, therefore, the higher offset from the base pointer in the stack frame.

The comment to encrypt says:

// Inputs: register EAX = 32-bit address of Ekey,
//                  ECX = the character to be encrypted

However, your calling sequence is:

movzx  eax, temp_char    ; push the char to encrypt FIRST
push   eax
lea    eax, EKey         ; push the encryption key SECOND
push   eax
call   encrypt

So the character is push first. So the character to encrypt But encrypt is loading them this way:

; On function entry, the old Instruction Pointer (4 bytes) is pushed onto the stack
; so now the EKey is +4 bytes from the stack pointer
; and the character is +8 bytes from the stack pointer
;

push ebp
mov ebp, esp

; We just pushed another 4 bytes onto the stack (the esp register)
; and THEN we put the stack pointer (esp) into ebp as base pointer
; to the stack frame.
;
; That means EKey is now +8 bytes off of the base pointer
; and the char to encrypt is +12 off of the base pointer
;
mov ecx, 8[ebp]            ; This loads EKey pointer to ECX
mov eax, 12[ebp]           ; This loads char-to-encrypt to EAX

The code then proceeds to try to reference EAX as a pointer (since it thinks that's EKey), which is going to cause an access violation since it's your character to encrypt the first time it tries to reference EAX as a pointer, which is here:

not byte ptr[eax]

So your debugger pointer was right! :)

You can fix it just by swapping these two registers:

mov eax, 8[ebp]            ; This loads EKey pointer to EAX
mov ecx, 12[ebp]           ; This loads char-to-encrypt to ECX

Finally, your call to encrypt doesn't clean up the stack pointer when it's done. Since you pushed 8 bytes of data onto the stack before calling encrypt, and since encrypt does a standard ret with no stack clean-up, you need to clean up after the call:

...
call   encrypt
add    esp, 8
...