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

[Kieran Bingham]

Hi Umang,

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());
> +	}

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

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