Cat program in x86 NASM

Question

I wrote cat program in x86 NASM and I would like to know if there is anything that can be improved.

This implementation of cat supports:

• Reading from stdin.
• Multiple arguments.

I checked few cases and program behaves exactly like original cat command.

I was thinking if I should add labels like write even if I wouldn't jmp to them, so code would be cleaner.

Code:

section .text
    global _start

_start:
    ;esi=1
    mov esi, 1

    ;if(argc==1)
    ;goto open
    cmp dword [esp], 1
    jnz open

    ;else
    ;fd=STDIN_FILENO
    mov dword [fd], 0
    jmp read

open:
    ;fd=open(path,oflag)
    mov eax, 5
    mov ebx, [esp+esi*4+4]
    mov ecx, 0
    int 0x80
    mov [fd], eax

read:
    ;bytes_read=read(fd,buf,BUFSIZE)
    mov eax, 3
    mov ebx, [fd]
    mov ecx, buf
    mov edx, BUFSIZE
    int 0x80

    ;write(STDOUT_FILENO,buf,bytes_read)
    mov edx, eax
    mov eax, 4
    mov ebx, 1
    mov ecx, buf
    int 0x80

    ;if(bytes_read!=0)
    ;goto read
    cmp edx, 0
    jnz read


    ;close(fd)
    mov eax, 6
    mov ebx, [fd]
    int 0x80

    ;esi++
    inc esi

    ;if(esi<argc)
    cmp dword esi, [esp]
    jl open

exit:
    mov eax, 1
    mov ebx, 0
    int 0x80

section .data
    BUFSIZE equ 1024

section .bss
    buf resb BUFSIZE
    fd resd 1

Answer 1

I was thinking if I should add labels like write even if I wouldn't jmp to them, so code would be cleaner.

Writing labels that aren't jumped to is fine. Provided they have meaningful names, it can help to understand the program.

My comments (not in a particular order)

;esi=1
mov esi, 1

This is a redundant comment! I already can see what the instruction does. What I can't know is what the ESI register will be used for.

;if(argc==1)
;goto open
cmp dword [esp], 1
jnz open

Here the comment does not correspond to the code. The code actually does if(argc!=1).

cmp edx, 0
jnz read

The more usual way to test for zero would be test edx, edx. It has a smaller encoding and is generally a bit faster.

mov ebx, 0

Similarly, zeroing a register is best done using the xor instruction (xor ebx, ebx). This is both faster and shorter.

mov dword [fd], 0

Your program would not need this instruction at all, if you would move the fd variable from the .bss section to the .data section using fd dd 0.

This in turn opens up an opportunity to shorter the code from:

    cmp dword [esp], 1
    jnz open
    mov dword [fd], 0
    jmp read
open:

to:

    cmp dword [esp], 1
    je  read
open:

Also note that in conjunction with the cmp instruction, it's better to use je and jne.
In conjunction with the test instruction, it's preferable to use jz and jnz.

je and jz have an identical encoding but choosing the right instruction mnemonic better expresses what the intention of the code is.

;if(esi<argc)
cmp dword esi, [esp]
jl open

The argc is by its very nature an unsigned number. After all it's just a count. You should use the conditional jumps that are provided to deal with the unsigned conditions. So better use jb open (JumpIfBelow).
Also there's no point in writing the dword size-tag. The mention of the dword register ESI already dictates the size.

mov edx, eax
mov eax, 4
mov ebx, 1
mov ecx, buf
int 0x80

I always prefer to write the function number directly above the int 0x80 instruction. That way it's immediately clear what function is getting invoked. And for perfection I even assign the registers in a sorted manner:

mov edx, eax
mov ecx, buf
mov ebx, 1
mov eax, 4
int 0x80

---

Putting it all together

• not being afraid to write redundant labels
• putting the comments in a separate column for readability
• shaving off one more byte by using the fact that ESI==1 when cmp dword [esp], 1 is executed.

section .text
    global _start

_start:
    mov  esi, 1
    cmp  [esp], esi        ;if(argc==1) goto read
    je   read
open:
    xor  ecx, ecx          ;fd=open(path,oflag)
    mov  ebx, [esp+esi*4+4]
    mov  eax, 5
    int  0x80
    mov  [fd], eax
read:
    mov  edx, BUFSIZE      ;bytes_read=read(fd,buf,BUFSIZE)
    mov  ecx, buf
    mov  ebx, [fd]
    mov  eax, 3
    int  0x80
write:
    mov  edx, eax          ;write(STDOUT_FILENO,buf,bytes_read)
    mov  ecx, buf
    mov  ebx, 1
    mov  eax, 4
    int  0x80
more:
    test edx, edx          ;if(bytes_read!=0) goto read
    jnz  read
close:
    mov  ebx, [fd]         ;close(fd)
    mov  eax, 6
    int  0x80
next:
    inc  esi
    cmp  esi, [esp]        ;if(esi<argc)
    jb   open
exit:
    xor  ebx, ebx
    mov  eax, 1
    int  0x80

section .data
    fd   dd 0              ;fd=STDIN_FILENO

section .bss
    BUFSIZE equ 1024
    buf  resb BUFSIZE

Concern

Nowhere in your code do you check for any errors from api calls like open or read. Imagine what might happen in these cases?