odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.17k stars 550 forks source link

Calling proc that has a 12 byte parameter causes stack buffer variable overflow #3159

Closed karl-zylinski closed 6 days ago

karl-zylinski commented 5 months ago

The follow program will, when compiled with odin run . -sanitize:address report a 16 byte write in a 12 byte region:

package bug_test

main :: proc() {
    test :: proc(something: int, thing1: Maybe([2]i32)) {
    }

    test(128, [2]i32{12,123})
}

At first I was trying to see if this was a false positive. But according to @laytan on Discord it seems like the IR is trying to store 2 x i64 into a { [2 x i32], i32 }. In this example I use a Maybe of [2]i32, which means 2xi32 plus i32 for the tag, i.e. 12 bytes, see first comment of the issue for a non-Maybe repro.

The IR is doing this both with and without ASAN address sanitizer enabled. Details of that discussion here: https://discord.com/channels/568138951836172421/585072813954564100/1202389269449023510

Failure Information (for bugs)

ASAN error output:

=================================================================
==13476==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffeb0e0f0a0 at pc 0x0000004dfb95 bp 0x7ffeb0e0f070 sp 0x7ffeb0e0f068
WRITE of size 16 at 0x7ffeb0e0f0a0 thread T0
    #0 0x4dfb94 in bug_test.main.test-0 odin_package
    #1 0x4dc9ce in bug_test.main odin_package
    #2 0x4df914 in main (/home/karl/repos/bugtest/bugtest+0x4df914) (BuildId: bfee972ef86a6adae440c1f116b63ef3637ba3ca)
    #3 0x7fb4c0429d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #4 0x7fb4c0429e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #5 0x41c304 in _start (/home/karl/repos/bugtest/bugtest+0x41c304) (BuildId: bfee972ef86a6adae440c1f116b63ef3637ba3ca)

Address 0x7ffeb0e0f0a0 is located in stack of thread T0 at offset 32 in frame
    #0 0x4dfabf in bug_test.main.test-0 odin_package

  This frame has 1 object(s):
    [32, 44) '' <== Memory access at offset 32 partially overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow odin_package in bug_test.main.test-0
Shadow bytes around the buggy address:
  0x1000561b9dc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9dd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9de0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9df0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1000561b9e10: f1 f1 f1 f1[00]04 f3 f3 00 00 00 00 00 00 00 00
  0x1000561b9e20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9e30: f1 f1 f1 f1 00 04 f3 f3 00 00 00 00 00 00 00 00
  0x1000561b9e40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9e50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000561b9e60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==13476==ABORTING

Steps to Reproduce

  1. Use Linux or MacOS (I haven't been able to see if ASAN reports this issue on windows as well, since address sanitizer is broken on windows with recent Visual Studio toolchains)
  2. Paste code snippet at top of report into a file
  3. Compile using odin build . -sanitize:address
karl-zylinski commented 5 months ago

A simpler repro without the Maybe is this:

package bug_test

TwelveByteStruct :: struct {
    thing1: i32,
    thing2: i32,
    thing3: i32,
}

main :: proc() {
    test :: proc(tbs: TwelveByteStruct) {
    }

    test({})
}

I think this means that any proc that takes a 12 byte param will cause a stack variable overflow.


Also "tarık b." on discord wrote about this a few weeks ago, which might be another variant of the same bug. It's also a 16 byte write into a 12 byte region... But when calling fmt.println(), I guess maybe the varargs are collected as a single block of memory and then written into the called procs stack, similar to the struct above?: (message link where they wrote this: https://discord.com/channels/568138951836172421/585072813954564100/1193291921414705203)

package prog
import "vendor:glfw"
import "core:fmt"
main :: proc() {
    major, minor, rev := glfw.GetVersion()
    fmt.printf("OpenGL version: %d.%d.%d\n", major, minor, rev)
}
==19165==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7f328d500020 at pc 0x000000506011 bp 0x7ffe9aae4230 sp 0x7ffe9aae4228
WRITE of size 16 at 0x7f328d500020 thread T0
    #0 0x506010 in prog.main odin_package
    #1 0x509044 in main (.../prog/prog+0x509044) (BuildId: 07bef459965573272c3f2a0ce6a5d421dd5406b3)
    #2 0x7f328f629d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #3 0x7f328f629e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #4 0x42a2f4 in _start (.../prog/prog+0x42a2f4) (BuildId: 07bef459965573272c3f2a0ce6a5d421dd5406b3)

Address 0x7f328d500020 is located in stack of thread T0 at offset 32 in frame
    #0 0x505f3f in prog.main odin_package

  This frame has 9 object(s):
    [32, 44) '' <== Memory access at offset 32 partially overflows this variable
    [64, 68) 'major'
    [80, 84) 'minor'
    [96, 100) 'rev'
    /*...*/
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow odin_package in prog.main
Shadow bytes around the buggy address:
/*...*/
=>0x7f328d500000: f1 f1 f1 f1[00]04 f3 f3 00 00 00 00 00 00 00 00
/*...*/
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==19165==ABORTING
laytan commented 5 months ago

Here is the offending IR:

%"bug_test.Maybe($T=[2]u32)-4372" = type { [2 x i32], i32 }

define internal void @bug_test.test-692([2 x i64] %0, ptr noalias nocapture nonnull %__.context_ptr) {
decls:
  %1 = alloca %"bug_test.Maybe($T=[2]u32)-4372", align 8
  br label %entry

entry:                                            ; preds = %decls
  store [2 x i64] %0, ptr %1, align 8

You can see that space is allocated for the maybe ({[2xi32], i32}), but [2xi64] is stored into it, I believe this triggers asan.

laytan commented 3 months ago

Not sure if it is fixed everywhere, but on macOS this was fixed in https://github.com/odin-lang/Odin/commit/c17adc98f5dfd313d4123c8a08d23eb1907e238f

gingerBill commented 3 months ago

This might be fixed now, please try again.

karl-zylinski commented 3 months ago

Hi, I tried to verify if it is fixed, but ASAN doesn't work anymore on my machine (Windows). Here's the output:

==10116==interception_win: unhandled instruction at 0x7ff71de3fc33: 4c 8d 15 c6 03 fb ff 49
AddressSanitizer: CHECK failed: sanitizer_common_interceptors_memintrinsics.inc:239 "((__interception::real_memcpy)) != (0)" (0x0, 0x0) (tid=19496)
    <empty stack>
JesseRMeyer commented 3 months ago

This is a problem between ASAN and Microsoft's C runtime: https://stackoverflow.com/questions/77557718/an-extremely-simple-program-triggers-an-unhandled-instruction-error-when-compi

karl-zylinski commented 2 weeks ago

I read the stuff on that link, but it seems outdated. So I still haven't been able to get ASAN working again on windows. If anyone has it working, then please test if if this is fixed so we can possibly close this.

JesseRMeyer commented 2 weeks ago

ASAN is only resolved on Windows when Odin is built with LLVM 18.

karl-zylinski commented 2 weeks ago

@JesseRMeyer Thank you. I'll try it whenever I have a version that uses LLVM 18.

karl-zylinski commented 6 days ago

I tried this on LLVM 18 now. It is fixed.