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

[Umang Jain]

Hi Kieran,

On 8/18/21 2:26 PM, Kieran Bingham wrote:
> 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'?

It's not fatal error (in practice), however it "looks" fatal to me. In 
practice, the test is completed and return with TestFailed / TestPassed 
values correctly (depending on whether you have locally applied Paul's 
patch)

Currently, the fd  leak test shall fail(on master) right? So ninja test 
will spit out the failure log.

You shall see the runtime_error in the failure log, provided you have 
enabled ASan during meson configure, otherwise not.

So, I suspect, the answer to your question is 'it depends' :-)

Once the Paul's fix is in with the leak test merged, you won't be able 
to see the runtime_error. Because the test shall pass via ninja test - 
and it won't print any log. Hence, it will keep happening(without this 
series) silently. One might need to check test logs in that case or run 
the test manually to see the runtime_error caught by ASan.

Does that help? Probably not, but this is the state I understand.


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