[Tinycc-devel] Please test repaired bounds-checking mode, especially on X86_64

[Kirill Smelkov]

(posting second time, after sibscribing to the list)

Hello tinycc'er,

I've fixed bounds checking mode and now btest passes on i386 and gcc-4.7
for me. The fixup involved touching X86_64 code though, which I have no
hardware to test, and also it would  be good to know whether bcheck now
works for other people, other OSes, etc...

The fixes are

    646b518 lib/bcheck: Prevent libc_malloc/libc_free etc from being miscompiled
    cffb7af lib/bcheck: Prevent __bound_local_new / __bound_local_delete from 
being miscompiled

and X86_64 people, could you please have an eye on the second patch - it
changes X86_64 assembly which I have no way to test - only guess my
changes are correct.

Thanks,
Kirill


---- 8< ----
commit cffb7af9f96834623ee0ff6b7fb10d56c91efb99
Author: Kirill Smelkov <address@hidden>
Date:   Tue Nov 13 13:14:26 2012 +0400

    lib/bcheck: Prevent __bound_local_new / __bound_local_delete from being 
miscompiled
    
    On i386 and gcc-4.7 I found that __bound_local_new was miscompiled -
    look:
    
        #ifdef __i386__
        /* return the frame pointer of the caller */
        #define GET_CALLER_FP(fp)\
        {\
            unsigned long *fp1;\
            __asm__ __volatile__ ("movl %%ebp,%0" :"=g" (fp1));\
            fp = fp1[0];\
        }
        #endif
    
        /* called when entering a function to add all the local regions */
        void FASTCALL __bound_local_new(void *p1)
        {
            unsigned long addr, size, fp, *p = p1;
            GET_CALLER_FP(fp);
            for(;;) {
                addr = p[0];
                if (addr == 0)
                    break;
                addr += fp;
                size = p[1];
                p += 2;
                __bound_new_region((void *)addr, size);
            }
        }
    
        __bound_local_new:
        .LFB40:
                .cfi_startproc
                pushl   %esi
                .cfi_def_cfa_offset 8
                .cfi_offset 6, -8
                pushl   %ebx
                .cfi_def_cfa_offset 12
                .cfi_offset 3, -12
                subl    $8, %esp            // NOTE prologue does not touch %ebp
                .cfi_def_cfa_offset 20
        #APP
        # 235 "lib/bcheck.c" 1
                movl %ebp,%edx              // %ebp -> fp1
        # 0 "" 2
        #NO_APP
                movl    (%edx), %esi        // fp1[0] -> fp
                movl    (%eax), %edx
                movl    %eax, %ebx
                testl   %edx, %edx
                je      .L167
                .p2align 2,,3
        .L173:
                movl    4(%ebx), %eax
                addl    $8, %ebx
                movl    %eax, 4(%esp)
                addl    %esi, %edx
                movl    %edx, (%esp)
                call    __bound_new_region
                movl    (%ebx), %edx
                testl   %edx, %edx
                jne     .L173
        .L167:
                addl    $8, %esp
                .cfi_def_cfa_offset 12
                popl    %ebx
                .cfi_restore 3
                .cfi_def_cfa_offset 8
                popl    %esi
                .cfi_restore 6
                .cfi_def_cfa_offset 4
                ret
    
    here GET_CALLER_FP() assumed that its using function setups it's stack
    frame, i.e. first save, then set %ebp to stack frame start, and then it
    has to do perform two lookups: 1) to get current stack frame through
    %ebp, and 2) get caller stack frame through (%ebp).
    
    And here is the problem: gcc decided not to setup %ebp for
    __bound_local_new and in such case GET_CALLER_FP actually becomes
    GET_CALLER_CALLER_FP and oops, wrong regions are registered in bcheck
    tables...
    
    The solution is to stop using hand written assembly and rely on gcc's
    __builtin_frame_address(1) to get callers frame stack(*). I think for the
    builtin gcc should generate correct code, independent of whether it
    decides or not to omit frame pointer in using function - it knows it.
    
    (*) judging by gcc history, __builtin_frame_address was there almost
        from the beginning - at least it is present in 1992 as seen from the
        following commit:
    
        
http://gcc.gnu.org/git/?p=gcc.git;a=commit;h=be07f7bdbac76d87d3006c89855491504d5d6202
    
        so we can rely on it being supported by all versions of gcc.
    
    In my environment the assembly of __bound_local_new changes as follows:
    
        diff --git a/bcheck0.s b/bcheck1.s
        index 4c02a5f..ef68918 100644
        --- a/bcheck0.s
        +++ b/bcheck1.s
        @@ -1409,20 +1409,17 @@ __bound_init:
         __bound_local_new:
         .LFB40:
                .cfi_startproc
        -       pushl   %esi
        +       pushl   %ebp                // NOTE prologue saves %ebp ...
                .cfi_def_cfa_offset 8
        -       .cfi_offset 6, -8
        +       .cfi_offset 5, -8
        +       movl    %esp, %ebp          // ... and reset it to local stack 
frame
        +       .cfi_def_cfa_register 5
        +       pushl   %esi
                pushl   %ebx
        -       .cfi_def_cfa_offset 12
        -       .cfi_offset 3, -12
                subl    $8, %esp
        -       .cfi_def_cfa_offset 20
        -#APP
        -# 235 "lib/bcheck.c" 1
        -       movl %ebp,%edx
        -# 0 "" 2
        -#NO_APP
        -       movl    (%edx), %esi
        +       .cfi_offset 6, -12
        +       .cfi_offset 3, -16
        +       movl    0(%ebp), %esi       // stkframe -> stkframe.parent -> fp
                movl    (%eax), %edx
                movl    %eax, %ebx
                testl   %edx, %edx
        @@ -1440,13 +1437,13 @@ __bound_local_new:
                jne     .L173
         .L167:
                addl    $8, %esp
        -       .cfi_def_cfa_offset 12
                popl    %ebx
                .cfi_restore 3
        -       .cfi_def_cfa_offset 8
                popl    %esi
                .cfi_restore 6
        -       .cfi_def_cfa_offset 4
        +       popl    %ebp
        +       .cfi_restore 5
        +       .cfi_def_cfa 4, 4
                ret
                .cfi_endproc
    
    i.e. now it compiles correctly.
    
    Though I do not have x86_64 to test, my guess is that
    __builtin_frame_address(1) should work there too. If not - please revert
    only x86_64 part of the patch. Thanks.
    
    Cc: Michael Matz <address@hidden>

diff --git a/lib/bcheck.c b/lib/bcheck.c
index 700ceda..5ea2d0b 100644
--- a/lib/bcheck.c
+++ b/lib/bcheck.c
@@ -208,25 +208,11 @@ BOUND_PTR_INDIR(8)
 BOUND_PTR_INDIR(12)
 BOUND_PTR_INDIR(16)
 
-#ifdef __i386__
 /* return the frame pointer of the caller */
 #define GET_CALLER_FP(fp)\
 {\
-    unsigned long *fp1;\
-    __asm__ __volatile__ ("movl %%ebp,%0" :"=g" (fp1));\
-    fp = fp1[0];\
+    fp = (unsigned long)__builtin_frame_address(1);\
 }
-#elif defined(__x86_64__)
-/* TCC always creates %rbp frames also on x86_64, so use them.  */
-#define GET_CALLER_FP(fp)\
-{\
-    unsigned long *fp1;\
-    __asm__ __volatile__ ("movq %%rbp,%0" :"=g" (fp1));\
-    fp = fp1[0];\
-}
-#else
-#error put code to extract the calling frame pointer
-#endif
 
 /* called when entering a function to add all the local regions */
 void FASTCALL __bound_local_new(void *p1)