Program to print all evironment variables

Question

I'm learning x86 assembly on Linux and this is the first program I wrote, so I'm looking for all kinds of ways to improve it.

; Build using these commands:
; nasm -f elf printenv.asm
; ld -o printenv printenv.o -m elf_i386 -s
section .data
section .bss
section .text
global _start
_start:
 pop ecx ; load argument count
 lea esp,[esp+ecx*4+4] ; move past every argument and null ptr
 PopEnv:
 pop esi ; get next env variable
 cmp esi,0 ; null means end
 je Exit ; exit if null
 ; calculate length
 mov ecx,0ffffffffh ; max length
 xor eax,eax ; search for null byte
 mov edi,esi ; where to look for
 repne scasb ; search
 dec edi ; edi points one past the null
 mov byte [edi],0ah ; replace null with EOL
 sub edi,esi ; length
 mov edx,edi ; set arg for sys_write
 inc edx ; include EOL
 ; print env to console
 mov eax,4 ; sys_write
 mov ebx,1 ; stdout
 mov ecx,esi ; location
 int 80h ; call
 cmp eax,edx ; check return value
 jb ExitFailure ; error
 jmp PopEnv ; get another env
 Exit:
 mov eax,1 ; sys_exit
 mov ebx,0 ; EXIT_SUCCESS
 int 80h ; make the call
 ExitFailure:
 mov eax,1 ; sys_exit
 mov ebx,-1 ; EXIT_FAILURE
 int 80h ; make the call

• syb0rg: Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information!

Answer 1

There's always a question of what 'improve' might mean. I don't see anything here that is 'wrong,' so mostly I'm commenting on 'style.' That said, if I were doing a code review, these are some of the things I'd mention:

• You have lots of very clear comments. That's a good thing, especially in assembler. I can't count the number of times I've read code and thought "What the heck was this guy THINKING?" This can be especially important when the code is wrong: He thinks it's doing x, but the code actually does y, so I am probably safe fixing it.
• You don't have a comment that describes what the program does. While your SE post has a (misspelled) description, the code does not.
• While it is not an issue for such a short program, you might want to start thinking in terms of 'routines.' Having a chunk of code that counts the length of a string (or that prints a string) that you can copy into your next program can be useful.
• While it works, I wouldn't get into the habit of using repne. It's not the best performing way to do this kind of loop (see https://stackoverflow.com/a/34141835/2189500). BTW, this is one of the reasons to use a high level language (like C). Compilers already know all this stuff and (try to) use the best possible construct.
• I expect that with a bit of fiddling (by using an offset on the mov) you can remove the dec edi/inc edx.
• I might consider changing the jb ExitFailure. jb can only jump over a certain amount of code, so your existing construct will work even if PopEnv was far away. But since your code is small, you could do:

a

jnb PopEnv ; get another env
ExitFailure:
mov eax,1 ; sys_exit
mov ebx,-1 ; EXIT_FAILURE
int 80h ; make the call
Exit:
mov eax,1 ; sys_exit
mov ebx,0 ; EXIT_SUCCESS
int 80h ; make the call

In summary, my comments are mostly just piddly 'style' stuff.

• Douglas: I timed the repne version vs the cmp and your suggestion was about 15% faster. Relocating the ExitFailure block was definitely much better. The only detail is that we should use jnl because the kernel returns negative values to indicate errors.