Secure reading function

[Grincheux]

These two functions seem to be secured. I would like to have your advice. What could I improve?

Code Select Expand


                            .686p                     ; create 32 bit code
                        .MMX                      ; enable MMX instructions
                        .XMM                      ; enable SSE instructions
                        .NOCREF
                        OPTION casemap :none      ; case sensitive

;    -------------------------------------------------------------------------------------------------------------
                        .Code
;    -------------------------------------------------------------------------------------------------------------

;    ==================================================================================
                        ALIGN    16
;    ==================================================================================

ReadWholeFile            PROC    __lpszFileName:LPSTR
                        LOCAL    _lpMemory:LPBYTE
                        LOCAL    _FileHandle:HANDLE
                        LOCAL    _dwFileSize:DWORD
                        LOCAL    _dwTmp:DWORD

                        INVOKE    PathFileExists,__lpszFileName

                        test    eax,eax
                        jz        @Error_01

                            INVOKE    CreateFile,__lpszFileName,GENERIC_READ,0,NULL,OPEN_EXISTING,FILE_ATTRIBUTE_NORMAL,NULL

                            cmp        eax,INVALID_HANDLE_VALUE
                            je        @Error_01

                                mov        _FileHandle,eax

                                INVOKE    GetFileSize,_FileHandle,ADDR _dwTmp

                                cmp        eax,INVALID_FILE_SIZE
                                je        @Error_02

                                    cmp        _dwTmp,0
                                    jne        @Error_02            ; File is too big

                                        or        eax,eax
                                        jz        @Error_02        ; File is empty

                                            mov        _dwFileSize,eax

                                            INVOKE    VirtualAlloc,NULL,eax,MEM_COMMIT + MEM_RESERVE,PAGE_EXECUTE_READWRITE

                                            test    eax,eax
                                            je        @Error_02

                                                mov        _lpMemory,eax
                                                lea        edx,_dwTmp

                                                INVOKE    ReadFile,_FileHandle,eax,_dwFileSize,edx,NULL

                                                test    eax,eax
                                                jz        @Error_03

                                                    mov        eax,_dwFileSize
                                                    sub        eax,_dwTmp
                                                    jnz        @Error_03

                                                        INVOKE    CloseHandle,_FileHandle

                                                        mov        eax,_lpMemory
                                                        ret

;    **********************************************************************************
                        ALIGN    16
;    **********************************************************************************

@Error_03 :        ; if eax = 0 we have reached the end of the file

                        push    MEM_RELEASE
                        push    0
                        push    _lpMemory
                        push    OFFSET @Error_02
                        jmp        VirtualFree

;    **********************************************************************************
                        ALIGN    16
;    **********************************************************************************

@Error_02 :

                        push    _FileHandle
                        push    OFFSET @Error_01
                        jmp        CloseHandle

;    **********************************************************************************
                        ALIGN    16
;    **********************************************************************************

@Error_01 :

                        xor        eax,eax
                        ret
ReadWholeFile            ENDP

;    ==================================================================================
                        ALIGN    16
;    ==================================================================================

File_Read                PROC    __FileHandle:HANDLE,__lpBuffer:LPBYTE,__dwNumberOfBytesToRead:DWord,__lpdwFlag:PTR DWord
                        LOCAL    _dwTmp:DWORD

                        lea        edx,_dwTmp

                        INVOKE    ReadFile,__FileHandle,__lpBuffer,__dwNumberOfBytesToRead,edx,NULL

                        test    eax,eax
                        jz        @Error_01

                            mov        eax,__dwNumberOfBytesToRead
                            sub        eax,_dwTmp
                            jnz        @Error_01

                                mov        eax,__lpdwFlag
                                mov        DWord Ptr [eax],TRUE            ; NO ERROR

                                mov        eax,__lpBuffer

                                ret

;    **********************************************************************************
                        ALIGN    16
;    **********************************************************************************

@Error_01 :

                        cmp        _dwTmp,0
                        jne        @Error

                            mov        eax,__lpdwFlag
                            mov        DWord Ptr [eax],FALSE                ; END OF FILE

                            mov        eax,__lpBuffer        ; End of file
                            ret                            ; That is not an error

@Error :

                        mov        eax,__lpdwFlag
                        mov        DWord Ptr [eax],-1                        ; ERROR WHILE READING

                        xor        eax,eax
                        ret
File_Read                ENDP


[TouEnMasm]

You use the exact size to allow memory,not very good (risk to read out of the memory ) .
Better is to round at least to the 16 bytes upper.
This avoid hole of memory (useless memory of a few bytes and win time)
You can use the fasted instructions using dword,qword,16 bytes registers.

[Grincheux]

I use VirtualAlloc, so the size is rounded to a multiple of 4096.
The case you describe could appened,  that's true.
Avoiding this case would obliged to add 4096. I could spend a lot of memory.

I found a bug in eof detection.
The EOF is detected when ReadFile returns TRUE and the number of bytes read is 0.

New version

[TouEnMasm]

To avoid the need of 4096 bytes,globalalloc or heapalloc are perfect.

[Grincheux]

Note :  The global functions have greater overhead and provide fewer features than other memory management functions. New applications should use the heap functions unless documentation states that a global function should be used.

But I prefer Virtual Functions.

I have added the SetErrorMode function with the parameter SEM_FAILCRITICALERRORS.
The attachment below is updated. Downloading this file always gives the last version.

[jj2007]

You can use VirtualAlloc, no problem. Just use filesize+1 to care for the zero delimiter. It will not waste memory.

Note, though, that
a) for small allocations VA is very slow compared to HeapAlloc
b) for big allocations (>0.5MB) HeapAlloc uses VirtualAlloc internally.
Therefore, to get always the best possible performance, you can use HeapAlloc. Microsoft says that, not me - I just explain why they are saying that ;-)

[Grincheux]

I try to use HeapAlloc :

Code Select Expand


add eax,256
push eax
push HEAP_ZERO_MEMORY + HEAP_NO_SERIALIZE
call GetProcessHeap
push eax
call HeapAlloc



Works well, but HeapFree returns 0!

Code Select Expand



push _lpMemory
push HEAP_NO_SERIALIZE
call GetProcessHeap
push eax
push OFFSET @ReadWholeFile_Error_02
jmp HeapFree


[dedndave]

HEAP_NO_SERIALIZE
This value should not be specified when accessing the process's default heap.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa366597%28v=vs.85%29.aspx

and, this is crazy code   

Code Select Expand

    push    OFFSET @ReadWholeFile_Error_02
    jmp     HeapFree

try this....

Code Select Expand

        INCLUDE    \Masm32\Include\Masm32rt.inc

;###############################################################################################

        .DATA?

hHeap   HANDLE ?
lpBlock LPVOID ?

;###############################################################################################

        .CODE

;***********************************************************************************************

main    PROC

        INVOKE  GetProcessHeap
        mov     hHeap,eax

        INVOKE  HeapAlloc,eax,HEAP_ZERO_MEMORY,120000
        mov     lpBlock,eax
        print   uhex$(eax),13,10,13,10

        inkey

        INVOKE  HeapFree,hHeap,NULL,lpBlock
        print   uhex$(eax),13,10,13,10

        inkey
        exit

main    ENDP

;###############################################################################################

        END     main

[Grincheux]

mov     lpBlock,eax[/size]

[/size]
[/size][size=78%]Rather than making this I made : MOV EAX,lpBlock[/size]
[/size][size=78%]So when I wanted to free the block the address was FALSE.[/size]

[Grincheux]

I am pleased to have finished this function!
The last and documented version is available here
Thanks everyone for your help. (Dedndae, Jj2007, ToutEnAsm)

[dedndave]

aligning the return addresses isn't really necessary...

Code Select Expand

push OFFSET @Error_02
jmp HeapFree

; **********************************************************************************
ALIGN 4
; **********************************************************************************

@Error_02 :

push _FileHandle
push OFFSET @Error_01
jmp CloseHandle

; **********************************************************************************
ALIGN 4
; **********************************************************************************

@Error_01 :

while it may speed up the RET (slightly), it is probably slower than CALL

[Grincheux]

In the last test JJ2007 did this code is quicker on AMD CPU

   push      _FileHandle
   push      OFFSET @Error_01
   jmp      CloseHandle

Agner Frog tells that if a call is followed by a jump, it can be replaced by a jump.
The is the answer to the "Crazy Code".

[dedndave]

yah - i don't really think that's valid
it's difficult to time an API function - let alone the return method
and - that's a lot of work to code each call that way
better to dereference the calls - lol

but, whatever floats your boat

[TouEnMasm]

interesting and improve is this one:
http://masm32.com/board/index.php?topic=3483.msg36676#msg36676
accept any types and any size of datas
wrong is the assertion:

If it is data that can be fitted into fixed length areas of memory, the task is simple, you allocate an array of pointers, then a block of memory and aim each pointer to each section of memory.

I  use it more often with lines of text (words) of various sizes (just add a zero at end before save them)
to empty the buffer is also easy:
invoke WriteToBlocHeap,addr BLOCHEAP,0,0  ;empty the block
a format can be also be used to save datas:
FORMAT STRUCT
       datalen DWORD
       thedata db datalen dup (?) ;pseudo code
FORMAT ENDS

this give:
mov edx,FORMAT.datalen
add edx,sizeof FORMAT.datalen (4)
invoke WriteToBlocHeap,addr BLOCHEAP,addr FORMAT,edx

[dedndave]

...and....

there are roughly 5 times as many intel cpu's running windows as there are amd   :P