James Molloy tutorial bug: Interrupt Handler ABI breakage

[Espanish]

Problem: Interrupt handlers corrupt interrupted state

Problem: Interrupt handlers corrupt interrupted state

This article previously told you to know the ABI. If you do you will see a huge problem in the interrupt.s suggested by the tutorial: It breaks the ABI for structure passing! It creates an instance of the struct registers on the stack and then passes it by value to the isr_handler function and then assumes the structure is intact afterwards. However, the function parameters on the stack belongs to the function and it is allowed to trash these values as it sees fit (if you need to know whether the compiler actually does this, you are thinking the wrong way, but it actually does). There are two ways around this. The most practical method is to pass the structure as a pointer instead, which allows you to explicitly edit the register state when needed - very useful for system calls, without having the compiler randomly doing it for you. The compiler can still edit the pointer on the stack when it's not specifically needed. The second option is to make another copy the structure and pass that.

First, I don't understand what the problem is. The original code works properly.
What is the document that explains why passing the registers structure by value is wrong?

Second, for us lazy people, how to fix this?
The Wiki suggests using a pointer to the structure. So then, we should push the address of the first element of the structure to the stack?

[sortie]

The code doesn't work properly. Don't be lazy.

I would explain the issue, but you actually just quoted what my explanation of what is wrong.

[Espanish]

I would explain the issue, but you actually just quoted what my explanation of what is wrong.

I would appreciate it if you could link to the official documentation that explains the proper way of doing things, or at least where you learned from, that made you realize why Molloy's code was incorrect.

In addition, I would ask you to consider adding sample code for 1) fixing the bug, 2) showing that it's actually there.

The other entries in this article are relatively straightforward to understand and to fix; this is less so, while at the same time it seems very important.

[max]

For official documentation, see http://wiki.osdev.org/System_V_ABI and the linked documents. Read it, understand it and you'll see the problem. Nope, no example code

[no92]

OSdev'ing requires you to read a lot. This means you can't be lazy and an OSdever at the same time. Force yourself to understand everything you encounter. That's the only way to understand it. In OSdeving, you have to strive for perfection, our you'll be bitten by your own code afterwards (that hurts!).

With average knowledge of assembly and the System V ABI, you should be able to understand it. You obviously failed to read the wiki and the System V ABI documentation.

[Espanish]

I have downloaded the file abi386-4.pdf and at page 44 it reads:

Structure and Union Arguments
As described in the data representation section, structures and unions can have
byte, halfword, or word alignment, depending on the constituents. An
argument’s size is increased, if necessary, to make it a multiple of words. This
may require tail padding, depending on the size of the argument. To ensure that
data in the stack is properly aligned, the stack pointer should always point to a
word boundary. Structure and union arguments are pushed onto the stack in the
same manner as integral arguments, described above. This provides call-by-value
semantics, letting the called function modify its arguments without affecting the
calling function’s object.

According to the above, Molloy's code should be correct. At the very least it does seem to work correctly.

[sortie]

The trouble isn't how you pass the structure. That is OK. The trouble is that you assume the parameters are intact afterwards when you restore the registers, but the ABI says that the called function can change the parameters on the stack if it wants to.

[Espanish]

Thanks, I think I understand now.
So the problem is that popa will restore messed up values if the handler function changed the contents of the registers structure.

In this case I have a very simple solution in mind: mark the parameter regs as const.
Is there a reason why this wouldn't be a good solution?

Code: Select all

void isr_handler(const registers_t regs);
void irq_handler(const registers_t regs);

[Octocontrabass]

Is there a reason why this wouldn't be a good solution?

Marking a parameter const only prevents the programmer from modifying it, not the compiler. The compiler "knows" parameters passed on the stack aren't used by the caller, so it can still modify them.

[sortie]

Code: Select all

int do_stuff(const int a)
{
     int b = other_stuff(a);
     more_stuff(&b);
     return b;
}

Let's look at this from a compiler point of view. a is a constant. a is only used once at the start and no further. The optimizer thus knows a doesn't need to be stored on the stack after its one use, so it can pop it from the stack. It now needs to store b somewhere, and notices the stack space already used by a is free, so it reuses it for b. The function finally returns. The variable a was constant, and it was never changed, but the memory it was stored in was changed. That's invisible to the C programmer, because the language behaves exactly as it was meant to. The rule is that the parameters to functions belong to the function, so the compiler doing this was allowed. The problem is that your assembly breaks this rule by thinking it remains constant. The const keyword won't save you, it works at the C language level, but it doesn't guarantee you that the stack space isn't reused.

The solution, obviously, is to make two copies of the registers. One that you will restore, and one to the called function might be corrupted. Alternatively, and much superior, you pass a pointer to the structure instead. Now the pointer will get corrupted, perhaps, but the registers themselves are preserved.

[Espanish]

Finally (fully) understanding the problem, I must say I'm surprised you spotted it. Nice catch.

As you suggested, I fixed it by passing a pointer to the structure, this involved pushing esp lastly.
It appears to work correctly. (Of course I also popped to ebx a second time, after the handler returned.)

[sortie]

Yeah, this register corruption bug was found because several established hobbyist kernels started having weird problems after changing seemingly innocent things. It turned out to be the ascendency from this tutorial that was the cause. It is quite unfortunate that bugs like this that require being an expert is hidden in the tutorial and hits even experienced developers, as the code looks very correct if you don't know that particular ABI rule.

Oh, and now that I think about it, his interrupt handler also doesn't 16-byte align the stack at the time of the call instruction. This is part of a GCC extension to the ABI where i386 stack calls are 16-byte aligned. This comes into play if you have stack variables aligned more than 4 byte. (Naturally, this stack alignment must also be ensured in a number of other places.)

As for an example on how I would do this. I'm slowly working on an example OS that shows the current best practices: https://gitorious.org/sortie/myos/

See these files:

• https://gitorious.org/sortie/myos/sourc ... pt-entry.S
• https://gitorious.org/sortie/myos/sourc ... nterrupt.c

[Octocontrabass]

This is part of a GCC extension to the ABI where i386 stack calls are 16-byte aligned.

Fortunately, there is also a GCC extension to make functions that will work without 16-byte alignment:

Code: Select all

__attribute__((force_align_arg_pointer))

I haven't seen the code this generates, so I don't know if it's any better or worse than Sortie's solution.