[libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run memcpy with null arguments

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Aug 18, 2021 at 09:56:44AM +0100, Kieran Bingham wrote:
> On 18/08/2021 09:38, Umang Jain wrote:
> > IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket
> > payload. However, if IPCMessage is constructor with one of the
> 
> s/constructor/constructed/
> 
> > following constructors -
> > 
> > 	IPCMessage::IPCMessage(),
> >         IPCMessage::IPCMessage(uint32_t cmd)
> >         IPCMessage::IPCMessage(const Header &header)
> 
> Different spacing on those lines, but it doesn't really matter.
> 
> > The data_ vector of IPCMessage is empty and uninitialised. In that
> > case, IPCMessage::payload will try to memcpy() empty data_ vector
> 
> ... to memcpy() an empty_ data vector ...
> 
> > which can lead to invoking memcpy() with nullptr. Add a non-empty
> 
> memcpy() with a nullptr parameter, which is then identified by the
> address sanity checker.
> 
> > data_ vector check to avoid it.
> > 
> > The issue is noticed by running a test manually, testing the vimc
> 
> Is it not noticed by running the unit tests with 'ninja test'?
> 
> If not - why not - why does it have to be run manually?
> 
> > IPA code paths in isolated mode. It is only noticed when the test
> > is compiled with -Db_sanitize=address,undefined meson built-in option.
> > 
> > ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/libcamera/ipc_pipe.cpp | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp
> > index 28e20e03..c8761320 100644
> > --- a/src/libcamera/ipc_pipe.cpp
> > +++ b/src/libcamera/ipc_pipe.cpp
> > @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const
> >  
> >  	memcpy(payload.data.data(), &header_, sizeof(Header));
> >  
> > -	/* \todo Make this work without copy */
> > -	memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());
> > +	if (data_.size() > 0) {
> > +		/* \todo Make this work without copy */
> > +		memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());

I'd wrap this as

		memcpy(payload.data.data() + sizeof(Header),
		       data_.data(), data_.size());

> > +	}
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +
> >  	payload.fds = fds_;
> >  
> >  	return payload;
> > 

-- 
Regards,

Laurent Pinchart