bounds checking with tcc

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

bounds checking with tcc

tinycc-devel mailing list
I tried bound checking with tcc and found some problems. (See attached
patch).

First the bounds checking code is included in shared objects and in the
application.
This means that for example malloc and friends are redirected twice.
I fixed this in tccelf.c

Second problem is that only arrays are checked.
But there are a lot of other objects like structs with arrays or
variables with address taken that should also work.
Fixed this in tccgen.c

The last one is some problems with code generation on x86_64.
The difficult one was the VT_LLOCAL code.
Fixed in x86_64-gen.c

I will stop now because bounds checking looks not to work for complex
applications.

Perhaps we could add the above code to git? The first and last one are
fixing problems.
For the last problem I have a testcase that reproduces the VT_LLOCAL
problem.

Regards,

     Herman

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

bounds (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Michael Matz-4
Hi,

On Tue, 29 Oct 2019, Herman ten Brugge via Tinycc-devel wrote:

> I tried bound checking with tcc and found some problems. (See attached
> patch).

The bound checking code is fairly incomplete, and some of it can't really
be fixed without more aggressive changes in TCC, my private opinion is
that we shouldn't really bother.  But some remarks on your changes:

> First the bounds checking code is included in shared objects and in the
> application.

You now made it so that libtcc1 is only included in the executables, never
in shared libs.  But that's wrong as well: e.g. if TCC generates a call to
__fixsfdi in a DLL the code for that wouldn't be included with the DLL
code anymore.  So in order to use that DLL you would now have to link
the application with TCCs libtcc1, and that only works by accident because
symbols from executables that are referred to from shared libs are
exported.  E.g. dlopening such shared lib wouldn't work anymore.

There are some possibilities to fix the problem correctly:
1) make the global libtcc1 symbols hidden
2) make the bound checking code (and only it) sit in a shared lib (in
   comparison to the current static lib), which is added to the requires
   of shared libs and executables

The former still means separate copies of the bounds checking code (and
hence separate data structures per DSO), but results in more
self-contained exectuables/libs.  The malloc hooks are still redirected
twice, so the code in bcheck.c simply would need to be prepared for this.

The second solution implies only one copy of the bounds infrastructure,
which might make some things easier, but leads to non-self-contained
executables/shared libs.

> This means that for example malloc and friends are redirected twice.
> I fixed this in tccelf.c
>
> Second problem is that only arrays are checked.
> But there are a lot of other objects like structs with arrays or variables
> with address taken that should also work.
> Fixed this in tccgen.c

But does this actually work?  Either the comment is obsolete, or it's not
that easy:

        /* XXX: currently, since we do only one pass, we cannot track
           '&' operators, so we add only arrays */
        if (bcheck && (type->t & VT_ARRAY)) {

The situation the comment alludes to is the following:

  void foo (void)
  {
    int locali;    // 0
    locali = 42;   // 1
    bar (&locali); // 2
    bla (locali);  // 3
  }

As TCC is single pass, at the declaration of locali you can't know that
eventually the address is taken.  So, as in your patch, you have to always
generate code assuming that the address will be eventually taken, i.e. for
all locals.  That's fairly wasteful in case the address is then not
actually taken.

> The last one is some problems with code generation on x86_64.
> The difficult one was the VT_LLOCAL code.
> Fixed in x86_64-gen.c

@@ -645,7 +645,11 @@ static void gen_static_call(int v)
 {
     Sym *sym = external_global_sym(v, &func_old_type);
     oad(0xe8, 0);
+#ifdef TCC_TARGET_PE
     greloca(cur_text_section, sym, ind-4, R_X86_64_PC32, -4);
+#else
+    greloca(cur_text_section, sym, ind-4, R_X86_64_PLT32, -4);
+#endif
 }

The function is named 'gen_static_call' and for that _PC32 really is the
correct relocation (well, actually it would be local calls, not only
static ones, of course).  The problem is that your change to not link
libtcc1.a into shared libs makes the calls to __bound_foo not be
static/local anymore.  So, the function would need to be renamed at least,
but see above why I think this ultimately is not a good idea.

Then the change for VT_LLOCAL:

             if ((r & VT_VALMASK) == VT_LLOCAL) {
                 SValue v1;
-                r = get_reg(RC_INT);
+                r = get_reg(RC_FLOAT);
                 v1.type.t = VT_PTR;
                 v1.r = VT_LOCAL | VT_LVAL;
                 v1.c.i = fc;
                 load(r, &v1);
                 fc = 0;
+                vtop->r = r = r | VT_LVAL;

Note quite: the register you use for loading the VT_LLOCAL slot must be
RC_INT (it's simply a pointer pointing to the ultimate lvalue).  I can see
why the addition of LVAL might be necessary sometimes, would be nice to
have a testcase.

> I will stop now because bounds checking looks not to work for complex
> applications.
>
> Perhaps we could add the above code to git? The first and last one are
> fixing problems. For the last problem I have a testcase that reproduces
> the VT_LLOCAL problem.

So, I'm against the libtcc1.a linking change; it might benefit bounds
checking right now, but worsens the situation for everything else.  The
VT_LLOCAL problem indeed should be fixed (but see above), especially if
you have a testcase.  The change to gen_static_call should be unnecessary
with the libtcc1 change.  To bounds-check all variables, not just arrays,
seems to be a fairly aggressive change, I think it's not really
appropriate at this time.


Ciao,
Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
On 2019-10-31 15:11, Michael Matz wrote:

> Hi,
>
> On Tue, 29 Oct 2019, Herman ten Brugge via Tinycc-devel wrote:
>
>> I tried bound checking with tcc and found some problems. (See attached
>> patch).
> The bound checking code is fairly incomplete, and some of it can't really
> be fixed without more aggressive changes in TCC, my private opinion is
> that we shouldn't really bother.  But some remarks on your changes:
>
>> First the bounds checking code is included in shared objects and in the
>> application.
> You now made it so that libtcc1 is only included in the executables, never
> in shared libs.  But that's wrong as well: e.g. if TCC generates a call to
> __fixsfdi in a DLL the code for that wouldn't be included with the DLL
> code anymore.  So in order to use that DLL you would now have to link
> the application with TCCs libtcc1, and that only works by accident because
> symbols from executables that are referred to from shared libs are
> exported.  E.g. dlopening such shared lib wouldn't work anymore.
>
> There are some possibilities to fix the problem correctly:
> 1) make the global libtcc1 symbols hidden
> 2) make the bound checking code (and only it) sit in a shared lib (in
>     comparison to the current static lib), which is added to the requires
>     of shared libs and executables
>
> The former still means separate copies of the bounds checking code (and
> hence separate data structures per DSO), but results in more
> self-contained exectuables/libs.  The malloc hooks are still redirected
> twice, so the code in bcheck.c simply would need to be prepared for this.
>
> The second solution implies only one copy of the bounds infrastructure,
> which might make some things easier, but leads to non-self-contained
> executables/shared libs.
>
>> This means that for example malloc and friends are redirected twice.
>> I fixed this in tccelf.c
Agreed. I won't fix this because bounds checking needs more changes.

>> Second problem is that only arrays are checked.
>> But there are a lot of other objects like structs with arrays or variables
>> with address taken that should also work.
>> Fixed this in tccgen.c
> But does this actually work?  Either the comment is obsolete, or it's not
> that easy:
>
>          /* XXX: currently, since we do only one pass, we cannot track
>             '&' operators, so we add only arrays */
>          if (bcheck && (type->t & VT_ARRAY)) {
>
> The situation the comment alludes to is the following:
>
>    void foo (void)
>    {
>      int locali;    // 0
>      locali = 42;   // 1
>      bar (&locali); // 2
>      bla (locali);  // 3
>    }
>
> As TCC is single pass, at the declaration of locali you can't know that
> eventually the address is taken.  So, as in your patch, you have to always
> generate code assuming that the address will be eventually taken, i.e. for
> all locals.  That's fairly wasteful in case the address is then not
> actually taken.
Agreed. But because tcc is one pass. There is no other solution I can
think off.
And yes this creates a lot of overhead.

>
>> The last one is some problems with code generation on x86_64.
>> The difficult one was the VT_LLOCAL code.
>> Fixed in x86_64-gen.c
> @@ -645,7 +645,11 @@ static void gen_static_call(int v)
>   {
>       Sym *sym = external_global_sym(v, &func_old_type);
>       oad(0xe8, 0);
> +#ifdef TCC_TARGET_PE
>       greloca(cur_text_section, sym, ind-4, R_X86_64_PC32, -4);
> +#else
> +    greloca(cur_text_section, sym, ind-4, R_X86_64_PLT32, -4);
> +#endif
>   }
>
> The function is named 'gen_static_call' and for that _PC32 really is the
> correct relocation (well, actually it would be local calls, not only
> static ones, of course).  The problem is that your change to not link
> libtcc1.a into shared libs makes the calls to __bound_foo not be
> static/local anymore.  So, the function would need to be renamed at least,
> but see above why I think this ultimately is not a good idea.
-- a.c --
extern void *memset(void *s, int c, unsigned long n);
void tst(void) { char a[10]; memset (a, 0, 10); }
-----
-- b.c --
extern void tst(void);
int main(void) { tst(); return 0; }
----
Compile with:
tcc -b -shared -o liba.so a.c
tcc -b b.c -o b liba.so
And then run with:
LD_LIBRARY_PATH=`pwd` ./b
I get:
./b: Symbol `__bound_init' causes overflow in R_X86_64_PC32 relocation
./b: Symbol `__bound_local_new' causes overflow in R_X86_64_PC32 relocation
./b: Symbol `__bound_local_delete' causes overflow in R_X86_64_PC32
relocation

This is fixed by this patch.

>
> Then the change for VT_LLOCAL:
>
>               if ((r & VT_VALMASK) == VT_LLOCAL) {
>                   SValue v1;
> -                r = get_reg(RC_INT);
> +                r = get_reg(RC_FLOAT);
>                   v1.type.t = VT_PTR;
>                   v1.r = VT_LOCAL | VT_LVAL;
>                   v1.c.i = fc;
>                   load(r, &v1);
>                   fc = 0;
> +                vtop->r = r = r | VT_LVAL;
>
> Note quite: the register you use for loading the VT_LLOCAL slot must be
> RC_INT (it's simply a pointer pointing to the ultimate lvalue).  I can see
> why the addition of LVAL might be necessary sometimes, would be nice to
> have a testcase.
See attached file. If I compile this with tcc -b. I get:
bcheck.c __bound_ptr_indir4: 0x7ffc6ed279b8 is outside of the region
This is fixed with the tccgen.c patch. See above.
After this the result printed is:
60 2
This should be
15 2
This is fixed by this patch. I Agree that the RC_INT -> RC_FLOAT is wrong.

Hope this explains things.

Regards,

     Herman

>> I will stop now because bounds checking looks not to work for complex
>> applications.
>>
>> Perhaps we could add the above code to git? The first and last one are
>> fixing problems. For the last problem I have a testcase that reproduces
>> the VT_LLOCAL problem.
> So, I'm against the libtcc1.a linking change; it might benefit bounds
> checking right now, but worsens the situation for everything else.  The
> VT_LLOCAL problem indeed should be fixed (but see above), especially if
> you have a testcase.  The change to gen_static_call should be unnecessary
> with the libtcc1 change.  To bounds-check all variables, not just arrays,
> seems to be a fairly aggressive change, I think it's not really
> appropriate at this time.
>
>
> Ciao,
> Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

f.c (426 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Michael Matz-4
Hello Herman,

On Tue, 26 Nov 2019, Herman ten Brugge wrote:

> > As TCC is single pass, at the declaration of locali you can't know that
> > eventually the address is taken.  So, as in your patch, you have to always
> > generate code assuming that the address will be eventually taken, i.e. for
> > all locals.  That's fairly wasteful in case the address is then not
> > actually taken.
>
> Agreed. But because tcc is one pass. There is no other solution I can
> think off. And yes this creates a lot of overhead.

Exactly.  So we need to make a decision if we want that extension of the
bounds-checking feature in TCC under these constraints.  In my opinion we
don't want to, but I won't stand in the way of extending it to locals (but
then the comment needs adjustment to talk about the expected larger
overhead for each local).

But the whole shared vs static lib support code needs resolving
nevertheless.

> > +#ifdef TCC_TARGET_PE
> >       greloca(cur_text_section, sym, ind-4, R_X86_64_PC32, -4);
> > +#else
> > +    greloca(cur_text_section, sym, ind-4, R_X86_64_PLT32, -4);
> > +#endif
> >   }
> >
> > The function is named 'gen_static_call' and for that _PC32 really is the
> > correct relocation (well, actually it would be local calls, not only
> > static ones, of course).  The problem is that your change to not link
> > libtcc1.a into shared libs makes the calls to __bound_foo not be
> > static/local anymore.  So, the function would need to be renamed at least,
> > but see above why I think this ultimately is not a good idea.
> -- a.c --
> extern void *memset(void *s, int c, unsigned long n);
> void tst(void) { char a[10]; memset (a, 0, 10); }
> -----
> -- b.c --
> extern void tst(void);
> int main(void) { tst(); return 0; }
> ----
> Compile with:
> tcc -b -shared -o liba.so a.c
> tcc -b b.c -o b liba.so
> And then run with:
> LD_LIBRARY_PATH=`pwd` ./b
> I get:
> ./b: Symbol `__bound_init' causes overflow in R_X86_64_PC32 relocation
> ./b: Symbol `__bound_local_new' causes overflow in R_X86_64_PC32 relocation
> ./b: Symbol `__bound_local_delete' causes overflow in R_X86_64_PC32 relocation
>
> This is fixed by this patch.
Sure, as I said, the overflowing relocations are a result of the
__bound_xxx functions now being included only in the executable, and hence
the calls from within the shared lib aren't within reach of 32bit offsets
anymore, so a PLT32 reloc needs to be used, and the call isn't local (or
static) anymore (but rather goes via the PLT).  So at the very least the
function name needs a change.  (The call to that function emitting
__chkstk on PE targets should remain local (i.e. PC32), though, which
happens with your patch).

But as I also said this patch is only necessary if the bounds checking
code is really moved into its separate shared library, and I don't think
that's the best idea, I'd like to have it stay in the static libtcc1.a,
with whatever other changes are necessary to support that (including being
prepared for multiple versions of that code), if at all possible.  (If
it's really not possible, then, well, we'll have to bite the apple or
declare the bounds checking code to not support shared libs).

> > Then the change for VT_LLOCAL:
> >
> >               if ((r & VT_VALMASK) == VT_LLOCAL) {
> >                   SValue v1;
> > -                r = get_reg(RC_INT);
> > +                r = get_reg(RC_FLOAT);
> >                   v1.type.t = VT_PTR;
> >                   v1.r = VT_LOCAL | VT_LVAL;
> >                   v1.c.i = fc;
> >                   load(r, &v1);
> >                   fc = 0;
> > +                vtop->r = r = r | VT_LVAL;
> >
> > Note quite: the register you use for loading the VT_LLOCAL slot must be
> > RC_INT (it's simply a pointer pointing to the ultimate lvalue).  I can see
> > why the addition of LVAL might be necessary sometimes, would be nice to
> > have a testcase.
> See attached file. If I compile this with tcc -b.
Thanks.  I can reproduce your findings.

> I get:
> bcheck.c __bound_ptr_indir4: 0x7ffc6ed279b8 is outside of the region
> This is fixed with the tccgen.c patch. See above.

Yeah.  We could also find some middle ground: only do the bounds stuff for
aggregates objects (nor just arrays as right now, or for all locals as
with your patch), i.e. also structs.  The TCC codegenerator takes address
of structs anyway to form member accesses, so that seems in line in what
we need.

> After this the result printed is:
> 60 2
> This should be
> 15 2
> This is fixed by this patch. I Agree that the RC_INT -> RC_FLOAT is wrong.

Indeed.  But some more things are wrong: even with your full bounds patch,
but moving the definition of 's' to file global level, the output is still
wrong.


Ciao,
Michael.

>
> Hope this explains things.
>
> Regards,
>
>     Herman
> >> I will stop now because bounds checking looks not to work for complex
> >> applications.
> >>
> >> Perhaps we could add the above code to git? The first and last one are
> >> fixing problems. For the last problem I have a testcase that reproduces
> >> the VT_LLOCAL problem.
> > So, I'm against the libtcc1.a linking change; it might benefit bounds
> > checking right now, but worsens the situation for everything else.  The
> > VT_LLOCAL problem indeed should be fixed (but see above), especially if
> > you have a testcase.  The change to gen_static_call should be unnecessary
> > with the libtcc1 change.  To bounds-check all variables, not just arrays,
> > seems to be a fairly aggressive change, I think it's not really
> > appropriate at this time.
> >
> >
> > Ciao,
> > Michael.
>
>
>
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
On 2019-11-27 18:17, Michael Matz wrote:

> Hello Herman,
>
> On Tue, 26 Nov 2019, Herman ten Brugge wrote:
>
>>> As TCC is single pass, at the declaration of locali you can't know that
>>> eventually the address is taken.  So, as in your patch, you have to always
>>> generate code assuming that the address will be eventually taken, i.e. for
>>> all locals.  That's fairly wasteful in case the address is then not
>>> actually taken.
>> Agreed. But because tcc is one pass. There is no other solution I can
>> think off. And yes this creates a lot of overhead.
> Exactly.  So we need to make a decision if we want that extension of the
> bounds-checking feature in TCC under these constraints.  In my opinion we
> don't want to, but I won't stand in the way of extending it to locals (but
> then the comment needs adjustment to talk about the expected larger
> overhead for each local).
I found some other code that uses the address off a local variable. See
attachment (c.c).
I can find more code like this :-(

>
> But the whole shared vs static lib support code needs resolving
> nevertheless.
I wanted to see how to fix the malloc/free wrappers. But now I see in
/usr/include/malloc.h:

/* Hooks for debugging and user-defined versions. */
extern void (*__MALLOC_HOOK_VOLATILE __free_hook) (void *__ptr,
                                                    const void *)
__MALLOC_DEPRECATED;
extern void *(*__MALLOC_HOOK_VOLATILE __malloc_hook)(size_t __size,
                                                      const void *)
__MALLOC_DEPRECATED;
extern void *(*__MALLOC_HOOK_VOLATILE __realloc_hook)(void *__ptr,
                                                       size_t __size,
                                                       const void *)
__MALLOC_DEPRECATED;
extern void *(*__MALLOC_HOOK_VOLATILE __memalign_hook)(size_t __alignment,
                                                        size_t __size,
                                                        const void *)

So these hooks are deprecated. We could fix this by including our own
malloc/free routines.
For a simple one see for example: https://github.com/mattconte/tlsf
This requires some time to do. But only solves one of the problems with
bounds-checking.

>
>>> +#ifdef TCC_TARGET_PE
>>>        greloca(cur_text_section, sym, ind-4, R_X86_64_PC32, -4);
>>> +#else
>>> +    greloca(cur_text_section, sym, ind-4, R_X86_64_PLT32, -4);
>>> +#endif
>>>    }
>>>
>>> The function is named 'gen_static_call' and for that _PC32 really is the
>>> correct relocation (well, actually it would be local calls, not only
>>> static ones, of course).  The problem is that your change to not link
>>> libtcc1.a into shared libs makes the calls to __bound_foo not be
>>> static/local anymore.  So, the function would need to be renamed at least,
>>> but see above why I think this ultimately is not a good idea.
>> -- a.c --
>> extern void *memset(void *s, int c, unsigned long n);
>> void tst(void) { char a[10]; memset (a, 0, 10); }
>> -----
>> -- b.c --
>> extern void tst(void);
>> int main(void) { tst(); return 0; }
>> ----
>> Compile with:
>> tcc -b -shared -o liba.so a.c
>> tcc -b b.c -o b liba.so
>> And then run with:
>> LD_LIBRARY_PATH=`pwd` ./b
>> I get:
>> ./b: Symbol `__bound_init' causes overflow in R_X86_64_PC32 relocation
>> ./b: Symbol `__bound_local_new' causes overflow in R_X86_64_PC32 relocation
>> ./b: Symbol `__bound_local_delete' causes overflow in R_X86_64_PC32 relocation
>>
>> This is fixed by this patch.
> Sure, as I said, the overflowing relocations are a result of the
> __bound_xxx functions now being included only in the executable, and hence
> the calls from within the shared lib aren't within reach of 32bit offsets
> anymore, so a PLT32 reloc needs to be used, and the call isn't local (or
> static) anymore (but rather goes via the PLT).  So at the very least the
> function name needs a change.  (The call to that function emitting
> __chkstk on PE targets should remain local (i.e. PC32), though, which
> happens with your patch).
>
> But as I also said this patch is only necessary if the bounds checking
> code is really moved into its separate shared library, and I don't think
> that's the best idea, I'd like to have it stay in the static libtcc1.a,
> with whatever other changes are necessary to support that (including being
> prepared for multiple versions of that code), if at all possible.  (If
> it's really not possible, then, well, we'll have to bite the apple or
> declare the bounds checking code to not support shared libs).
>
>>> Then the change for VT_LLOCAL:
>>>
>>>                if ((r & VT_VALMASK) == VT_LLOCAL) {
>>>                    SValue v1;
>>> -                r = get_reg(RC_INT);
>>> +                r = get_reg(RC_FLOAT);
>>>                    v1.type.t = VT_PTR;
>>>                    v1.r = VT_LOCAL | VT_LVAL;
>>>                    v1.c.i = fc;
>>>                    load(r, &v1);
>>>                    fc = 0;
>>> +                vtop->r = r = r | VT_LVAL;
>>>
>>> Note quite: the register you use for loading the VT_LLOCAL slot must be
>>> RC_INT (it's simply a pointer pointing to the ultimate lvalue).  I can see
>>> why the addition of LVAL might be necessary sometimes, would be nice to
>>> have a testcase.
>> See attached file. If I compile this with tcc -b.
> Thanks.  I can reproduce your findings.
>
>> I get:
>> bcheck.c __bound_ptr_indir4: 0x7ffc6ed279b8 is outside of the region
>> This is fixed with the tccgen.c patch. See above.
> Yeah.  We could also find some middle ground: only do the bounds stuff for
> aggregates objects (nor just arrays as right now, or for all locals as
> with your patch), i.e. also structs.  The TCC codegenerator takes address
> of structs anyway to form member accesses, so that seems in line in what
> we need.
>
>> After this the result printed is:
>> 60 2
>> This should be
>> 15 2
>> This is fixed by this patch. I Agree that the RC_INT -> RC_FLOAT is wrong.
> Indeed.  But some more things are wrong: even with your full bounds patch,
> but moving the definition of 's' to file global level, the output is still
> wrong.
You just found another bounds checking problem :-) I fixed this with
some push/pop trickery. This probably could be improved.
I have now added a minimum patch so bounds checking works
a little bit.
We need still to fix the shared lib reloc problems and the malloc/free
hooks.
Probably we also need to track the address off so code runs faster.

Regards,

     Herman

>
>
> Ciao,
> Michael.
>> Hope this explains things.
>>
>> Regards,
>>
>>      Herman
>>>> I will stop now because bounds checking looks not to work for complex
>>>> applications.
>>>>
>>>> Perhaps we could add the above code to git? The first and last one are
>>>> fixing problems. For the last problem I have a testcase that reproduces
>>>> the VT_LLOCAL problem.
>>> So, I'm against the libtcc1.a linking change; it might benefit bounds
>>> checking right now, but worsens the situation for everything else.  The
>>> VT_LLOCAL problem indeed should be fixed (but see above), especially if
>>> you have a testcase.  The change to gen_static_call should be unnecessary
>>> with the libtcc1 change.  To bounds-check all variables, not just arrays,
>>> seems to be a fairly aggressive change, I think it's not really
>>> appropriate at this time.
>>>
>>>
>>> Ciao,
>>> Michael.
>>

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

c.c (253 bytes) Download Attachment
patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Michael Matz-4
Hello,

On Wed, 27 Nov 2019, Herman ten Brugge wrote:

> > Exactly.  So we need to make a decision if we want that extension of the
> > bounds-checking feature in TCC under these constraints.  In my opinion we
> > don't want to, but I won't stand in the way of extending it to locals (but
> > then the comment needs adjustment to talk about the expected larger
> > overhead for each local).
> I found some other code that uses the address off a local variable. See
> attachment (c.c).
> I can find more code like this :-(

Exactly my point.  bounds-checking in TCC is a cute hack, but nothing
more.  With other compilers gaining capabilities like sanitizers, plus
valgrind and friends, I think the value of that feature in tcc
asymptotically goes to zero.

> So these hooks are deprecated. We could fix this by including our own
> malloc/free routines. For a simple one see for example:
> https://github.com/mattconte/tlsf

Yeah, well, there are many allocators out there, simple and complex.  I
don't think we should include any for this half-broken feature, especially
because it doesn't really help.  You would have to rewrite all calls to
malloc/free/realloc in the sources you see to redirect to the new
allocator.  The sources you don't see can't be rewritten, so any pointers
you get from libraries can't be bounds-checked.  That's the whole point of
using the malloc hooks, you can intercept _all_ calls to the allocation
routines, even from foreign libraries.  Without hooks you aren't going to
be reliable.

Of course the same can be achieved on ELF platforms via symbol
interposition.  And once you do that, you don't need hooks anymore, but
you also don't need a custom allocator.

> > Indeed.  But some more things are wrong: even with your full bounds patch,
> > but moving the definition of 's' to file global level, the output is still
> > wrong.
> You just found another bounds checking problem :-)

Exactly :)

> I fixed this with some push/pop trickery. This probably could be
> improved. I have now added a minimum patch so bounds checking works a
> little bit. We need still to fix the shared lib reloc problems and the
> malloc/free hooks.

Do we?  Can we perhaps also simply declare bounds checking to work only
with the main executable?  Or remove that whole feature altogether?

> Probably we also need to track the address off so
> code runs faster.

I don't see a way for that except giving up on the single pass nature of
TCC, at which point it wouldn't be a tiny C compiler anymore.


Ciao,
Michael.


>
> Regards,
>
>     Herman
> >
> >
> > Ciao,
> > Michael.
> >> Hope this explains things.
> >>
> >> Regards,
> >>
> >>      Herman
> >>>> I will stop now because bounds checking looks not to work for complex
> >>>> applications.
> >>>>
> >>>> Perhaps we could add the above code to git? The first and last one are
> >>>> fixing problems. For the last problem I have a testcase that reproduces
> >>>> the VT_LLOCAL problem.
> >>> So, I'm against the libtcc1.a linking change; it might benefit bounds
> >>> checking right now, but worsens the situation for everything else.  The
> >>> VT_LLOCAL problem indeed should be fixed (but see above), especially if
> >>> you have a testcase.  The change to gen_static_call should be unnecessary
> >>> with the libtcc1 change.  To bounds-check all variables, not just arrays,
> >>> seems to be a fairly aggressive change, I think it's not really
> >>> appropriate at this time.
> >>>
> >>>
> >>> Ciao,
> >>> Michael.
> >>
>
>
>
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Michael Matz-4
Hello again,

but to maybe be a bit more constructive:

On Thu, 28 Nov 2019, Michael Matz wrote:

> > I fixed this with some push/pop trickery.

I see, yeah, expanding calls during calls is broken as gfunc_call in the
generators doesn't generally leave a trace in vtop[] which registers are
currently holding values.  I think you only need so push/pop si/di, as
cx/dx aren't used intentionally during reg-param setup.

(I think i386-gen.c has a simila bug with fastcall functions).

> > This probably could be
> > improved. I have now added a minimum patch so bounds checking works a
> > little bit. We need still to fix the shared lib reloc problems and the
> > malloc/free hooks.
>
> Do we?  Can we perhaps also simply declare bounds checking to work only
> with the main executable?  Or remove that whole feature altogether?

And perhaps another compromise: only conditionally enable tracking of
locals: Invent a new cmdline option (say, '-bb'), which sets
do_bounds_checking to 2.  And only if it's > 1 you would also track
locals, whereas with == 1 you would only track arrays and structs.

Your decision, I think you can push this patch either with that change, or
without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
source code larger or uglier in any meaningful way, but does fix practical
bugs.


Ciao,
Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
In reply to this post by Michael Matz-4
Hello,

On 2019-11-28 17:08, Michael Matz wrote:

> Hello,
>
> On Wed, 27 Nov 2019, Herman ten Brugge wrote:
>
>>> Exactly.  So we need to make a decision if we want that extension of the
>>> bounds-checking feature in TCC under these constraints.  In my opinion we
>>> don't want to, but I won't stand in the way of extending it to locals (but
>>> then the comment needs adjustment to talk about the expected larger
>>> overhead for each local).
>> I found some other code that uses the address off a local variable. See
>> attachment (c.c).
>> I can find more code like this :-(
> Exactly my point.  bounds-checking in TCC is a cute hack, but nothing
> more.  With other compilers gaining capabilities like sanitizers, plus
> valgrind and friends, I think the value of that feature in tcc
> asymptotically goes to zero.
>
>> So these hooks are deprecated. We could fix this by including our own
>> malloc/free routines. For a simple one see for example:
>> https://github.com/mattconte/tlsf
> Yeah, well, there are many allocators out there, simple and complex.  I
> don't think we should include any for this half-broken feature, especially
> because it doesn't really help.  You would have to rewrite all calls to
> malloc/free/realloc in the sources you see to redirect to the new
> allocator.  The sources you don't see can't be rewritten, so any pointers
> you get from libraries can't be bounds-checked.  That's the whole point of
> using the malloc hooks, you can intercept _all_ calls to the allocation
> routines, even from foreign libraries.  Without hooks you aren't going to
> be reliable.
>
> Of course the same can be achieved on ELF platforms via symbol
> interposition.  And once you do that, you don't need hooks anymore, but
> you also don't need a custom allocator.
>
>>> Indeed.  But some more things are wrong: even with your full bounds patch,
>>> but moving the definition of 's' to file global level, the output is still
>>> wrong.
>> You just found another bounds checking problem :-)
> Exactly :)
>
>> I fixed this with some push/pop trickery. This probably could be
>> improved. I have now added a minimum patch so bounds checking works a
>> little bit. We need still to fix the shared lib reloc problems and the
>> malloc/free hooks.
> Do we?  Can we perhaps also simply declare bounds checking to work only
> with the main executable?  Or remove that whole feature altogether?
>
>> Probably we also need to track the address off so
>> code runs faster.
> I don't see a way for that except giving up on the single pass nature of
> TCC, at which point it wouldn't be a tiny C compiler anymore.
>
After this long discussion I think the best thing to do is to remove the
bounds checking feature from tcc. As you mentioned there are much
better tools available today. Even allowing this only for the main
executable still gives problems (see f.c test code).
So fixing this for tcc will take quite some effort.

I can create a patch to remove bounds checking if you want.

A separate patch is the VT_LLOCAL change in x86_64-gen.c.
It will be a challenge to write a testcase for this.

Regards,

     Herman


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
In reply to this post by Michael Matz-4
On 2019-11-28 17:41, Michael Matz wrote:

> Hello again,
>
> but to maybe be a bit more constructive:
>
> On Thu, 28 Nov 2019, Michael Matz wrote:
>
>>> I fixed this with some push/pop trickery.
> I see, yeah, expanding calls during calls is broken as gfunc_call in the
> generators doesn't generally leave a trace in vtop[] which registers are
> currently holding values.  I think you only need so push/pop si/di, as
> cx/dx aren't used intentionally during reg-param setup.
>
> (I think i386-gen.c has a simila bug with fastcall functions).
I did this on purpose. The register calling usage is rdi, rsi, rdx, rcx.
So If a call uses four parameters the rdx, rcx would be overwritten
by the bounds checking code.
The bound_ptr_indirx calls overwrite rdi, rsi, rdx and rcx.

>
>>> This probably could be
>>> improved. I have now added a minimum patch so bounds checking works a
>>> little bit. We need still to fix the shared lib reloc problems and the
>>> malloc/free hooks.
>> Do we?  Can we perhaps also simply declare bounds checking to work only
>> with the main executable?  Or remove that whole feature altogether?
> And perhaps another compromise: only conditionally enable tracking of
> locals: Invent a new cmdline option (say, '-bb'), which sets
> do_bounds_checking to 2.  And only if it's > 1 you would also track
> locals, whereas with == 1 you would only track arrays and structs.
>
> Your decision, I think you can push this patch either with that change, or
> without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
> source code larger or uglier in any meaningful way, but does fix practical
> bugs.
I can make a patch for this and as well.

Currently I think the best thing to do is to remove it. Other tools
do a much better job.

If you still want to keep the code we probably should add some warnings.
For example do not use it for shared libraries.

Regards,

     Herman



_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Christian Jullien-3
Hello friends,
First I must say that I never used tcc bound checking because, as Michael said, I mostly use valgrind and other gcc/clang analysers when code security matters in devel cycle.
The only possible interesting use I see is when tcc is used natively on Windows when no other C compiler is installed.

I will not complain if this feature is removed.

C.

-----Original Message-----
From: Tinycc-devel [mailto:tinycc-devel-bounces+eligis=[hidden email]] On Behalf Of Herman ten Brugge via Tinycc-devel
Sent: Thursday, November 28, 2019 18:16
To: Michael Matz
Cc: Herman ten Brugge; Herman ten Brugge via Tinycc-devel
Subject: Re: [Tinycc-devel] bounds checking with tcc

On 2019-11-28 17:41, Michael Matz wrote:

> Hello again,
>
> but to maybe be a bit more constructive:
>
> On Thu, 28 Nov 2019, Michael Matz wrote:
>
>>> I fixed this with some push/pop trickery.
> I see, yeah, expanding calls during calls is broken as gfunc_call in the
> generators doesn't generally leave a trace in vtop[] which registers are
> currently holding values.  I think you only need so push/pop si/di, as
> cx/dx aren't used intentionally during reg-param setup.
>
> (I think i386-gen.c has a simila bug with fastcall functions).
I did this on purpose. The register calling usage is rdi, rsi, rdx, rcx.
So If a call uses four parameters the rdx, rcx would be overwritten
by the bounds checking code.
The bound_ptr_indirx calls overwrite rdi, rsi, rdx and rcx.

>
>>> This probably could be
>>> improved. I have now added a minimum patch so bounds checking works a
>>> little bit. We need still to fix the shared lib reloc problems and the
>>> malloc/free hooks.
>> Do we?  Can we perhaps also simply declare bounds checking to work only
>> with the main executable?  Or remove that whole feature altogether?
> And perhaps another compromise: only conditionally enable tracking of
> locals: Invent a new cmdline option (say, '-bb'), which sets
> do_bounds_checking to 2.  And only if it's > 1 you would also track
> locals, whereas with == 1 you would only track arrays and structs.
>
> Your decision, I think you can push this patch either with that change, or
> without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
> source code larger or uglier in any meaningful way, but does fix practical
> bugs.
I can make a patch for this and as well.

Currently I think the best thing to do is to remove it. Other tools
do a much better job.

If you still want to keep the code we probably should add some warnings.
For example do not use it for shared libraries.

Regards,

     Herman



_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
I tried -b on windows for this simple code:
------
#include <stdio.h>

int
main (void)
{
   int i;
   int a[10];

   for (i = 0; i < 11; i++) {
     a[i] = i;
     printf ("%d %d\n", i, a[i]);
   }
   return 0;
}
---
It did not report an error as it should and printed wrong values. So
windows is probably broken too.

Regards,

     Herman


On 2019-11-28 18:52, Christian Jullien wrote:

> Hello friends,
> First I must say that I never used tcc bound checking because, as Michael said, I mostly use valgrind and other gcc/clang analysers when code security matters in devel cycle.
> The only possible interesting use I see is when tcc is used natively on Windows when no other C compiler is installed.
>
> I will not complain if this feature is removed.
>
> C.
>
> -----Original Message-----
> From: Tinycc-devel [mailto:tinycc-devel-bounces+eligis=[hidden email]] On Behalf Of Herman ten Brugge via Tinycc-devel
> Sent: Thursday, November 28, 2019 18:16
> To: Michael Matz
> Cc: Herman ten Brugge; Herman ten Brugge via Tinycc-devel
> Subject: Re: [Tinycc-devel] bounds checking with tcc
>
> On 2019-11-28 17:41, Michael Matz wrote:
>> Hello again,
>>
>> but to maybe be a bit more constructive:
>>
>> On Thu, 28 Nov 2019, Michael Matz wrote:
>>
>>>> I fixed this with some push/pop trickery.
>> I see, yeah, expanding calls during calls is broken as gfunc_call in the
>> generators doesn't generally leave a trace in vtop[] which registers are
>> currently holding values.  I think you only need so push/pop si/di, as
>> cx/dx aren't used intentionally during reg-param setup.
>>
>> (I think i386-gen.c has a simila bug with fastcall functions).
> I did this on purpose. The register calling usage is rdi, rsi, rdx, rcx.
> So If a call uses four parameters the rdx, rcx would be overwritten
> by the bounds checking code.
> The bound_ptr_indirx calls overwrite rdi, rsi, rdx and rcx.
>>>> This probably could be
>>>> improved. I have now added a minimum patch so bounds checking works a
>>>> little bit. We need still to fix the shared lib reloc problems and the
>>>> malloc/free hooks.
>>> Do we?  Can we perhaps also simply declare bounds checking to work only
>>> with the main executable?  Or remove that whole feature altogether?
>> And perhaps another compromise: only conditionally enable tracking of
>> locals: Invent a new cmdline option (say, '-bb'), which sets
>> do_bounds_checking to 2.  And only if it's > 1 you would also track
>> locals, whereas with == 1 you would only track arrays and structs.
>>
>> Your decision, I think you can push this patch either with that change, or
>> without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
>> source code larger or uglier in any meaningful way, but does fix practical
>> bugs.
> I can make a patch for this and as well.
>
> Currently I think the best thing to do is to remove it. Other tools
> do a much better job.
>
> If you still want to keep the code we probably should add some warnings.
> For example do not use it for shared libraries.
>
> Regards,
>
>       Herman
>
>
>
> _______________________________________________
> Tinycc-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/tinycc-devel
>


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
In reply to this post by Michael Matz-4
Hello,

I did some debugging with bouds-checking and came up with attached patch.
I seriously doubt any one did use bounds checking in a large project before.

Currently I can use this now in a large multi threaded project. It still
needs some more testing so do not apply the patch yet.

I disabled some errors. For example if a bounded pointer is not found I
give no error. I also relaxed printing free errors.
There were some off by 1 errors in lib/bcheck.c and I needed to make the
code thread safe.
I used the patch to not link in libtcc1.a in shared objects when bounds
checking so I have only one memory pool.
This has to be documented because you cannot use this with dlopen for
example.
I also added the pthread library when bounds checking so it is now multi
threaded.
I found another problem with nocode_wanted when using sizeof().
Also the push/pop trick needed to push some more registers when more
parameters are passed in registers.

I probably forget to mention a lot a other changes. See the patch.

I only tested this on linux x86_64. There are for sure problems on other
targets.

Regards,

     Herman


On 2019-11-28 17:41, Michael Matz wrote:

> Hello again,
>
> but to maybe be a bit more constructive:
>
> On Thu, 28 Nov 2019, Michael Matz wrote:
>
>>> I fixed this with some push/pop trickery.
> I see, yeah, expanding calls during calls is broken as gfunc_call in the
> generators doesn't generally leave a trace in vtop[] which registers are
> currently holding values.  I think you only need so push/pop si/di, as
> cx/dx aren't used intentionally during reg-param setup.
>
> (I think i386-gen.c has a simila bug with fastcall functions).
>
>>> This probably could be
>>> improved. I have now added a minimum patch so bounds checking works a
>>> little bit. We need still to fix the shared lib reloc problems and the
>>> malloc/free hooks.
>> Do we?  Can we perhaps also simply declare bounds checking to work only
>> with the main executable?  Or remove that whole feature altogether?
> And perhaps another compromise: only conditionally enable tracking of
> locals: Invent a new cmdline option (say, '-bb'), which sets
> do_bounds_checking to 2.  And only if it's > 1 you would also track
> locals, whereas with == 1 you would only track arrays and structs.
>
> Your decision, I think you can push this patch either with that change, or
> without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
> source code larger or uglier in any meaningful way, but does fix practical
> bugs.
>
>
> Ciao,
> Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
Little updated patch. Still needs more work.

Regards,

     Herman

On 2019-12-02 18:24, Herman ten Brugge wrote:

> Hello,
>
> I did some debugging with bouds-checking and came up with attached patch.
> I seriously doubt any one did use bounds checking in a large project
> before.
>
> Currently I can use this now in a large multi threaded project. It
> still needs some more testing so do not apply the patch yet.
>
> I disabled some errors. For example if a bounded pointer is not found
> I give no error. I also relaxed printing free errors.
> There were some off by 1 errors in lib/bcheck.c and I needed to make
> the code thread safe.
> I used the patch to not link in libtcc1.a in shared objects when
> bounds checking so I have only one memory pool.
> This has to be documented because you cannot use this with dlopen for
> example.
> I also added the pthread library when bounds checking so it is now
> multi threaded.
> I found another problem with nocode_wanted when using sizeof().
> Also the push/pop trick needed to push some more registers when more
> parameters are passed in registers.
>
> I probably forget to mention a lot a other changes. See the patch.
>
> I only tested this on linux x86_64. There are for sure problems on
> other targets.
>
> Regards,
>
>     Herman
>
>
> On 2019-11-28 17:41, Michael Matz wrote:
>> Hello again,
>>
>> but to maybe be a bit more constructive:
>>
>> On Thu, 28 Nov 2019, Michael Matz wrote:
>>
>>>> I fixed this with some push/pop trickery.
>> I see, yeah, expanding calls during calls is broken as gfunc_call in the
>> generators doesn't generally leave a trace in vtop[] which registers are
>> currently holding values.  I think you only need so push/pop si/di, as
>> cx/dx aren't used intentionally during reg-param setup.
>>
>> (I think i386-gen.c has a simila bug with fastcall functions).
>>
>>>> This probably could be
>>>> improved. I have now added a minimum patch so bounds checking works a
>>>> little bit. We need still to fix the shared lib reloc problems and the
>>>> malloc/free hooks.
>>> Do we?  Can we perhaps also simply declare bounds checking to work only
>>> with the main executable?  Or remove that whole feature altogether?
>> And perhaps another compromise: only conditionally enable tracking of
>> locals: Invent a new cmdline option (say, '-bb'), which sets
>> do_bounds_checking to 2.  And only if it's > 1 you would also track
>> locals, whereas with == 1 you would only track arrays and structs.
>>
>> Your decision, I think you can push this patch either with that
>> change, or
>> without (but try to remove cx/dx from the push/pop).  It doesn't make
>> tccs
>> source code larger or uglier in any meaningful way, but does fix
>> practical
>> bugs.
>>
>>
>> Ciao,
>> Michael.
>

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

tinycc-devel mailing list
I think I have a working patch now. See attachment.
The code runs on i386 and x86_64 on linux for large projects I have.

I found some bugs when updating the i386 code.
The bugs are in the testcases in the patch.
The test/boundtest.c code works for both targets.
See below for the main changes.

I probably need some feedback now.

Regards,

     Herman

On 2019-12-02 22:46, Herman ten Brugge wrote:

> Little updated patch. Still needs more work.
>
> Regards,
>
>     Herman
>
> On 2019-12-02 18:24, Herman ten Brugge wrote:
>> Hello,
>>
>> I did some debugging with bouds-checking and came up with attached
>> patch.
>> I seriously doubt any one did use bounds checking in a large project
>> before.
>>
>> Currently I can use this now in a large multi threaded project. It
>> still needs some more testing so do not apply the patch yet.
>>
>> I disabled some errors. For example if a bounded pointer is not found
>> I give no error. I also relaxed printing free errors.
>> There were some off by 1 errors in lib/bcheck.c and I needed to make
>> the code thread safe.
>> I used the patch to not link in libtcc1.a in shared objects when
>> bounds checking so I have only one memory pool.
>> This has to be documented because you cannot use this with dlopen for
>> example.
>> I also added the pthread library when bounds checking so it is now
>> multi threaded.
>> I found another problem with nocode_wanted when using sizeof().
>> Also the push/pop trick needed to push some more registers when more
>> parameters are passed in registers.
>>
>> I probably forget to mention a lot a other changes. See the patch.
>>
>> I only tested this on linux x86_64. There are for sure problems on
>> other targets.
>>
>> Regards,
>>
>>     Herman
>>
>>
>> On 2019-11-28 17:41, Michael Matz wrote:
>>> Hello again,
>>>
>>> but to maybe be a bit more constructive:
>>>
>>> On Thu, 28 Nov 2019, Michael Matz wrote:
>>>
>>>>> I fixed this with some push/pop trickery.
>>> I see, yeah, expanding calls during calls is broken as gfunc_call in
>>> the
>>> generators doesn't generally leave a trace in vtop[] which registers
>>> are
>>> currently holding values.  I think you only need so push/pop si/di, as
>>> cx/dx aren't used intentionally during reg-param setup.
>>>
>>> (I think i386-gen.c has a simila bug with fastcall functions).
>>>
>>>>> This probably could be
>>>>> improved. I have now added a minimum patch so bounds checking works a
>>>>> little bit. We need still to fix the shared lib reloc problems and
>>>>> the
>>>>> malloc/free hooks.
>>>> Do we?  Can we perhaps also simply declare bounds checking to work
>>>> only
>>>> with the main executable?  Or remove that whole feature altogether?
>>> And perhaps another compromise: only conditionally enable tracking of
>>> locals: Invent a new cmdline option (say, '-bb'), which sets
>>> do_bounds_checking to 2.  And only if it's > 1 you would also track
>>> locals, whereas with == 1 you would only track arrays and structs.
>>>
>>> Your decision, I think you can push this patch either with that
>>> change, or
>>> without (but try to remove cx/dx from the push/pop).  It doesn't
>>> make tccs
>>> source code larger or uglier in any meaningful way, but does fix
>>> practical
>>> bugs.
>>>
>>>
>>> Ciao,
>>> Michael.
>>
>

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

patch (62K) Download Attachment
ian
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

ian
In reply to this post by tinycc-devel mailing list

Hi.

Wondering why you think a mailing-list is a review/tests review notepad.
Not under-rating your work, it's very fine, but why not a private chat ?
Or only when tested a simple comment on a fork ?

Btw, I'd be glad to test it on x86 32 bits linux 5.0.

Regards, ian

Le 02/12/2019 à 22:46, Herman ten Brugge via Tinycc-devel a écrit :
Little updated patch. Still needs more work.

Regards,

    Herman

On 2019-12-02 18:24, Herman ten Brugge wrote:
Hello,

I did some debugging with bouds-checking and came up with attached patch.
I seriously doubt any one did use bounds checking in a large project before.

Currently I can use this now in a large multi threaded project. It still needs some more testing so do not apply the patch yet.

I disabled some errors. For example if a bounded pointer is not found I give no error. I also relaxed printing free errors.
There were some off by 1 errors in lib/bcheck.c and I needed to make the code thread safe.
I used the patch to not link in libtcc1.a in shared objects when bounds checking so I have only one memory pool.
This has to be documented because you cannot use this with dlopen for example.
I also added the pthread library when bounds checking so it is now multi threaded.
I found another problem with nocode_wanted when using sizeof().
Also the push/pop trick needed to push some more registers when more parameters are passed in registers.

I probably forget to mention a lot a other changes. See the patch.

I only tested this on linux x86_64. There are for sure problems on other targets.

Regards,

    Herman


On 2019-11-28 17:41, Michael Matz wrote:
Hello again,

but to maybe be a bit more constructive:

On Thu, 28 Nov 2019, Michael Matz wrote:

I fixed this with some push/pop trickery.
I see, yeah, expanding calls during calls is broken as gfunc_call in the
generators doesn't generally leave a trace in vtop[] which registers are
currently holding values.  I think you only need so push/pop si/di, as
cx/dx aren't used intentionally during reg-param setup.

(I think i386-gen.c has a simila bug with fastcall functions).

This probably could be
improved. I have now added a minimum patch so bounds checking works a
little bit. We need still to fix the shared lib reloc problems and the
malloc/free hooks.
Do we?  Can we perhaps also simply declare bounds checking to work only
with the main executable?  Or remove that whole feature altogether?
And perhaps another compromise: only conditionally enable tracking of
locals: Invent a new cmdline option (say, '-bb'), which sets
do_bounds_checking to 2.  And only if it's > 1 you would also track
locals, whereas with == 1 you would only track arrays and structs.

Your decision, I think you can push this patch either with that change, or
without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
source code larger or uglier in any meaningful way, but does fix practical
bugs.


Ciao,
Michael.



_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
--
-- [hidden email]
-- Développeur compulsif

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: bounds checking with tcc

Michael Matz-4
In reply to this post by tinycc-devel mailing list
Hello,

On Wed, 4 Dec 2019, Herman ten Brugge wrote:

> I think I have a working patch now. See attachment.
> The code runs on i386 and x86_64 on linux for large projects I have.
>
> I found some bugs when updating the i386 code.
> The bugs are in the testcases in the patch.
> The test/boundtest.c code works for both targets.
> See below for the main changes.
>
> I probably need some feedback now.

I'll try to look at it over the weekend.


Ciao,
Michael.

>
> Regards,
>
>     Herman
>
> On 2019-12-02 22:46, Herman ten Brugge wrote:
> > Little updated patch. Still needs more work.
> >
> > Regards,
> >
> >     Herman
> >
> > On 2019-12-02 18:24, Herman ten Brugge wrote:
> >> Hello,
> >>
> >> I did some debugging with bouds-checking and came up with attached patch.
> >> I seriously doubt any one did use bounds checking in a large project
> >> before.
> >>
> >> Currently I can use this now in a large multi threaded project. It still
> >> needs some more testing so do not apply the patch yet.
> >>
> >> I disabled some errors. For example if a bounded pointer is not found I
> >> give no error. I also relaxed printing free errors.
> >> There were some off by 1 errors in lib/bcheck.c and I needed to make the
> >> code thread safe.
> >> I used the patch to not link in libtcc1.a in shared objects when bounds
> >> checking so I have only one memory pool.
> >> This has to be documented because you cannot use this with dlopen for
> >> example.
> >> I also added the pthread library when bounds checking so it is now multi
> >> threaded.
> >> I found another problem with nocode_wanted when using sizeof().
> >> Also the push/pop trick needed to push some more registers when more
> >> parameters are passed in registers.
> >>
> >> I probably forget to mention a lot a other changes. See the patch.
> >>
> >> I only tested this on linux x86_64. There are for sure problems on other
> >> targets.
> >>
> >> Regards,
> >>
> >>     Herman
> >>
> >>
> >> On 2019-11-28 17:41, Michael Matz wrote:
> >>> Hello again,
> >>>
> >>> but to maybe be a bit more constructive:
> >>>
> >>> On Thu, 28 Nov 2019, Michael Matz wrote:
> >>>
> >>>>> I fixed this with some push/pop trickery.
> >>> I see, yeah, expanding calls during calls is broken as gfunc_call in the
> >>> generators doesn't generally leave a trace in vtop[] which registers are
> >>> currently holding values.  I think you only need so push/pop si/di, as
> >>> cx/dx aren't used intentionally during reg-param setup.
> >>>
> >>> (I think i386-gen.c has a simila bug with fastcall functions).
> >>>
> >>>>> This probably could be
> >>>>> improved. I have now added a minimum patch so bounds checking works a
> >>>>> little bit. We need still to fix the shared lib reloc problems and the
> >>>>> malloc/free hooks.
> >>>> Do we?  Can we perhaps also simply declare bounds checking to work only
> >>>> with the main executable?  Or remove that whole feature altogether?
> >>> And perhaps another compromise: only conditionally enable tracking of
> >>> locals: Invent a new cmdline option (say, '-bb'), which sets
> >>> do_bounds_checking to 2.  And only if it's > 1 you would also track
> >>> locals, whereas with == 1 you would only track arrays and structs.
> >>>
> >>> Your decision, I think you can push this patch either with that change, or
> >>> without (but try to remove cx/dx from the push/pop).  It doesn't make tccs
> >>> source code larger or uglier in any meaningful way, but does fix practical
> >>> bugs.
> >>>
> >>>
> >>> Ciao,
> >>> Michael.
> >>
> >
>
>
>
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel