Re: [Tinycc-devel] bounds checking with tcc

[Michael Matz]

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.