[libcamera-devel] [PATCH/RFC] libcamera: bound_method: Please gcc undefined behaviour sanitizer

[Laurent Pinchart]

Hi Kieran,

On Tue, Apr 13, 2021 at 02:02:46PM +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> $SUBJECT might need improving ;-)

s/Please/Please the/ ?

> On 12/04/2021 23:59, Laurent Pinchart wrote:
> > Enabling the gcc undefined behaviour sanitizer (with the meson configure
> > -Db_sanitize=undefined option) causes many tests to fail, with errors
> > such as the following (for test/object-invoke):
> > 
> > ../../include/libcamera/bound_method.h:198:27: runtime error: member access within address 0x55fcd7bfbd38 which does not point to an object of type 'BoundMethodBase'
> > 0x55fcd7bfbd38: note: object has invalid vptr
> >  fc 55 00 00  2a 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  31 00 00 00 00 00 00 00  4b c6 72 88
> >               ^~~~~~~~~~~~~~~~~~~~~~~
> >               invalid vptr
> > ../../include/libcamera/bound_method.h:198:41: runtime error: member call on null pointer of type 'struct InvokedObject'
> > ../../include/libcamera/bound_method.h:198:41: runtime error: member access within null pointer of type 'struct InvokedObject'
> > Segmentation fault
> > 
> > The root cause isn't clear, but this change fixes the issue. It may be a
> > bug in gcc.
> 
> Given that this is just a small semantic change to separate the casting
> of the object, and the calling, I don't think there's any harm in
> applying this as is, given that even if we fix GCC we'd have to wait for
> it to filter down?

I agree.

> However I have no real understanding of the underlying issues either I'm
> afraid, so I don't think I can help ...
> 
> Is it easy to reduce it down to a reproducible issue?

It's lots of template code, so it may not be that easy. I haven't tried
yet. Ideally we should file a bug with gcc.

> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/bound_method.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > If anyone is interested in trying the gcc undefined behaviour sanitizer,
> > this patch is needed. Bonus points if you can spot why it helps.
> > 
> > diff --git a/include/libcamera/bound_method.h b/include/libcamera/bound_method.h
> > index f216e3b56826..de17fdee3612 100644
> > --- a/include/libcamera/bound_method.h
> > +++ b/include/libcamera/bound_method.h
> > @@ -163,7 +163,8 @@ public:
> >  
> >  	R invoke(Args... args) override
> >  	{
> > -		return (static_cast<T *>(this->obj_)->*func_)(args...);
> > +		T *obj = static_cast<T *>(this->obj_);
> > +		return (obj->*func_)(args...);
> >  	}
> >  
> >  private:
> > @@ -195,7 +196,8 @@ public:
> >  
> >  	void invoke(Args... args) override
> >  	{
> > -		(static_cast<T *>(this->obj_)->*func_)(args...);
> > +		T *obj = static_cast<T *>(this->obj_);
> > +		(obj->*func_)(args...);
> >  	}
> >  
> >  private:
> > 

-- 
Regards,

Laurent Pinchart