Re: [Tinycc-devel] bounds checking with tcc

[Michael Matz]

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.
> 
> 
>