apsun / loliOS

Lightweight & operational Linux-inspired OS.
33 stars 1 forks source link

Fix stack corruption with -mrtd (stdcall) #26

Closed apsun closed 2 years ago

apsun commented 2 years ago

Seems to only be exhibited when compiling with clang with optimizations enabled.

apsun commented 2 years ago

This seems to repro:

#include <string.h>

typedef struct { char buf[128]; } foo_t;

__attribute__((noinline))
static void bar(foo_t *f)
{
    f->buf[42] = 69;
}

int main(void)
{
    foo_t f = {0};
    bar(&f);
    return f.buf[42];
}

Disassembly is suspicious:

08049000 <main>:
 8049000:       55                      push   %ebp
 8049001:       89 e5                   mov    %esp,%ebp
 8049003:       56                      push   %esi
 8049004:       81 ec 84 00 00 00       sub    $0x84,%esp
 804900a:       83 ec 04                sub    $0x4,%esp
 804900d:       8d b5 78 ff ff ff       lea    -0x88(%ebp),%esi
 8049013:       68 80 00 00 00          push   $0x80
 8049018:       6a 00                   push   $0x0
 804901a:       56                      push   %esi
 804901b:       e8 8f 23 00 00          call   804b3af <memset>
 8049020:       83 c4 04                add    $0x4,%esp
 8049023:       56                      push   %esi
 8049024:       e8 10 00 00 00          call   8049039 <bar>
 8049029:       83 c4 0c                add    $0xc,%esp
 804902c:       0f be 46 2a             movsbl 0x2a(%esi),%eax
 8049030:       81 c4 84 00 00 00       add    $0x84,%esp
 8049036:       5e                      pop    %esi
 8049037:       5d                      pop    %ebp
 8049038:       c3                      ret    

Seems that it's calling memset with cdecl calling convention, even though the actual memset is compiled as stdcall due to -mrtd. I suspect this is some crazy interaction with the compiler builtin functions.

apsun commented 2 years ago

This patch does seem to fix the issue... but at what cost? I think this is the wrong path to go down, we should instead fix the stupid memcpy/memmove/memset synthesis (related to #25)

apsun commented 2 years ago

Okay, seems that this is a behavior difference between GCC and clang

GCC intrinsic libcalls respect the global -mrtd flag, whereas clang doesn't. Therefore, the "fix" to mark memset/memcpy/memmove as cdecl would technically break GCC instead. However, it seems that GCC doesn't generate libcalls if -ffreestanding is specified, so... maybe cdecl is the way to go?

Or we can go in the other direction, maybe it's not worth the time and we should just drop support for everything other than cdecl. Or clang. Or both.

apsun commented 2 years ago

Since #25 is fixed, I think the policy for now will be that -mrtd is unsupported. I'll keep writing __attribute__((cdecl)) to document that something is callable from asm, and maybe to occasionally test -mrtd with GCC.