tinycc-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Tinycc-devel] bounds checking with tcc


From: Herman ten Brugge
Subject: Re: [Tinycc-devel] bounds checking with tcc
Date: Wed, 27 Nov 2019 21:30:18 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

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.


Attachment: c.c
Description: Text Data

Attachment: patch
Description: Text document


reply via email to

[Prev in Thread] Current Thread [Next in Thread]