On Thu, Oct 23, 2014 at 07:41:12PM +0400, Ilya Verbin wrote:
> > malloc can fail, SIGSEGV in response to that is not desirable.
> > Can't you fallback to alloca, or use just alloca, or use alloca
> > with malloc fallback?
>
> I replaced it with alloca.
There is a risk if a suid or otherwise priviledge escalated program
uses it and attacker passes huge env vars.
Perhaps use alloca if it is <= 2KB and malloc otherwise, and in that case
if malloc fails, just do a fatal error?
> > Where does this artificial limit come from? Using libNNN.so library names?
> > Can't you use lib%d.so instead?
>
> Yes, it comes from the Image structure
> (liboffloadmic/runtime/offload_host.h:52)
> It must contain a null-terminated name, therefore I need to allocate some
> space
> for the name in plugin's struct TargetImage. But the structure can't contain
> any bytes after the trailing zero and before the actual data.
> So, now I extended the name to 10 digits and removed the comparison with 1000.
Ok.
> > Also, seeing register_image, shouldn't there be
> > GOMP_OFFLOAD_unregister_image which would be invoked when the library
> > containing MIC offloading regions is dlclosed?
> > One could use __cxa_atexit or similar for that, something that is given
> > &__dso_handle. Or is no cleanup necessary? At least unregistering it
> > from translation tables, because the same addresses might be reused by a
> > different shared library?
> > With dlopen/dlclose in mind, 1000 might be easily reached, consider 10000
> > times dlopening/dlclosing (perhaps over longer time, by long running daemon)
> > a shared library containg #pragma omp target region.
>
> Hmm, previously we've tested only cases when all libraries are loaded before
> the
> first offload. Offloading from a dlopened library after the call to
> gomp_target_init isn't working. So, this will require some changes in
> libgomp/target.c . Is it ok to fix this bug in a separate patch?
I guess it can be done incrementally, even during stage3.
Jakub
