[{"id":18899,"web_url":"https://patchwork.libcamera.org/comment/18899/","msgid":"<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","date":"2021-08-18T08:56:44","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 18/08/2021 09:38, Umang Jain wrote:\n> IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket\n> payload. However, if IPCMessage is constructor with one of the\n\ns/constructor/constructed/\n\n> following constructors -\n> \n> \tIPCMessage::IPCMessage(),\n>         IPCMessage::IPCMessage(uint32_t cmd)\n>         IPCMessage::IPCMessage(const Header &header)\n\nDifferent spacing on those lines, but it doesn't really matter.\n\n> The data_ vector of IPCMessage is empty and uninitialised. In that\n> case, IPCMessage::payload will try to memcpy() empty data_ vector\n\n... to memcpy() an empty_ data vector ...\n\n> which can lead to invoking memcpy() with nullptr. Add a non-empty\n\nmemcpy() with a nullptr parameter, which is then identified by the\naddress sanity checker.\n\n> data_ vector check to avoid it.\n> \n> The issue is noticed by running a test manually, testing the vimc\n\nIs it not noticed by running the unit tests with 'ninja test'?\n\nIf not - why not - why does it have to be run manually?\n\n\n> IPA code paths in isolated mode. It is only noticed when the test\n> is compiled with -Db_sanitize=address,undefined meson built-in option.\n> \n> ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  src/libcamera/ipc_pipe.cpp | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n> index 28e20e03..c8761320 100644\n> --- a/src/libcamera/ipc_pipe.cpp\n> +++ b/src/libcamera/ipc_pipe.cpp\n> @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const\n>  \n>  \tmemcpy(payload.data.data(), &header_, sizeof(Header));\n>  \n> -\t/* \\todo Make this work without copy */\n> -\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> +\tif (data_.size() > 0) {\n> +\t\t/* \\todo Make this work without copy */\n> +\t\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> +\t}\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> +\n>  \tpayload.fds = fds_;\n>  \n>  \treturn payload;\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EC2C1BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 08:56:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64C9B68895;\n\tWed, 18 Aug 2021 10:56:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FFD66025E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 10:56:47 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DA07466;\n\tWed, 18 Aug 2021 10:56:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PySHVIsN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629277006;\n\tbh=EqG1O225zmdexAj87La26p28EyL5t39F4UEIIpNAe3I=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=PySHVIsNINAAIPpltPhgoT4kRm1+0e/Inkx8OxvPLdyzg+BuveTUSTF54Q0BDnsTV\n\tyc8NMQ3gaPpSQW04mv1mETOTpWswd5uaxvmfZNFEpXiUXw7oq1XxW/141ltxfzCWrz\n\tAjcz9R4cXZUd5gqegDS+mwDvLRMvS1U7SFG41dJ4=","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210818083842.31778-1-umang.jain@ideasonboard.com>\n\t<20210818083842.31778-3-umang.jain@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","Date":"Wed, 18 Aug 2021 09:56:44 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210818083842.31778-3-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18902,"web_url":"https://patchwork.libcamera.org/comment/18902/","msgid":"<24a4bfe6-8fc0-34e8-a034-03e7114d8a5b@ideasonboard.com>","date":"2021-08-18T09:14:54","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 8/18/21 2:26 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 18/08/2021 09:38, Umang Jain wrote:\n>> IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket\n>> payload. However, if IPCMessage is constructor with one of the\n> s/constructor/constructed/\n>\n>> following constructors -\n>>\n>> \tIPCMessage::IPCMessage(),\n>>          IPCMessage::IPCMessage(uint32_t cmd)\n>>          IPCMessage::IPCMessage(const Header &header)\n> Different spacing on those lines, but it doesn't really matter.\n>\n>> The data_ vector of IPCMessage is empty and uninitialised. In that\n>> case, IPCMessage::payload will try to memcpy() empty data_ vector\n> ... to memcpy() an empty_ data vector ...\n>\n>> which can lead to invoking memcpy() with nullptr. Add a non-empty\n> memcpy() with a nullptr parameter, which is then identified by the\n> address sanity checker.\n>\n>> data_ vector check to avoid it.\n>>\n>> The issue is noticed by running a test manually, testing the vimc\n> Is it not noticed by running the unit tests with 'ninja test'?\n\nIt's not fatal error (in practice), however it \"looks\" fatal to me. In \npractice, the test is completed and return with TestFailed / TestPassed \nvalues correctly (depending on whether you have locally applied Paul's \npatch)\n\nCurrently, the fd  leak test shall fail(on master) right? So ninja test \nwill spit out the failure log.\n\nYou shall see the runtime_error in the failure log, provided you have \nenabled ASan during meson configure, otherwise not.\n\nSo, I suspect, the answer to your question is 'it depends' :-)\n\nOnce the Paul's fix is in with the leak test merged, you won't be able \nto see the runtime_error. Because the test shall pass via ninja test - \nand it won't print any log. Hence, it will keep happening(without this \nseries) silently. One might need to check test logs in that case or run \nthe test manually to see the runtime_error caught by ASan.\n\nDoes that help? Probably not, but this is the state I understand.\n\n\n>\n> If not - why not - why does it have to be run manually?\n>\n>\n>> IPA code paths in isolated mode. It is only noticed when the test\n>> is compiled with -Db_sanitize=address,undefined meson built-in option.\n>>\n>> ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null\n>>\n>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>> ---\n>>   src/libcamera/ipc_pipe.cpp | 7 +++++--\n>>   1 file changed, 5 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n>> index 28e20e03..c8761320 100644\n>> --- a/src/libcamera/ipc_pipe.cpp\n>> +++ b/src/libcamera/ipc_pipe.cpp\n>> @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const\n>>   \n>>   \tmemcpy(payload.data.data(), &header_, sizeof(Header));\n>>   \n>> -\t/* \\todo Make this work without copy */\n>> -\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n>> +\tif (data_.size() > 0) {\n>> +\t\t/* \\todo Make this work without copy */\n>> +\t\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n>> +\t}\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n>> +\n>>   \tpayload.fds = fds_;\n>>   \n>>   \treturn payload;\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1448BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 09:15:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42BC168895;\n\tWed, 18 Aug 2021 11:15:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB69A6025E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 11:15:01 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C5E1466;\n\tWed, 18 Aug 2021 11:15:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XNVXPr8y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629278101;\n\tbh=/wLxO+HQht9/hva6sHkuf20wGcaQ6ArflvDbFkX5i6g=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=XNVXPr8yy1+ruzQ1Yvzpl7TWAvYzQmyVtu5bKBYMAYLV44U5UMWhMCsprswQswV/R\n\tbPhTpwIVR33+kjc6lOVauZgXet3r/7qHjhhWHoyRh4z5gmRZD1ewcgLsHU7Oxhx5S9\n\tcMd1oKDXHB8qhB2+6TDUKVqWbRsZImxKaTTZyw5Q=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210818083842.31778-1-umang.jain@ideasonboard.com>\n\t<20210818083842.31778-3-umang.jain@ideasonboard.com>\n\t<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<24a4bfe6-8fc0-34e8-a034-03e7114d8a5b@ideasonboard.com>","Date":"Wed, 18 Aug 2021 14:44:54 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18923,"web_url":"https://patchwork.libcamera.org/comment/18923/","msgid":"<YR07LNT57V3WTjYQ@pendragon.ideasonboard.com>","date":"2021-08-18T16:54:04","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Aug 18, 2021 at 09:56:44AM +0100, Kieran Bingham wrote:\n> On 18/08/2021 09:38, Umang Jain wrote:\n> > IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket\n> > payload. However, if IPCMessage is constructor with one of the\n> \n> s/constructor/constructed/\n> \n> > following constructors -\n> > \n> > \tIPCMessage::IPCMessage(),\n> >         IPCMessage::IPCMessage(uint32_t cmd)\n> >         IPCMessage::IPCMessage(const Header &header)\n> \n> Different spacing on those lines, but it doesn't really matter.\n> \n> > The data_ vector of IPCMessage is empty and uninitialised. In that\n> > case, IPCMessage::payload will try to memcpy() empty data_ vector\n> \n> ... to memcpy() an empty_ data vector ...\n> \n> > which can lead to invoking memcpy() with nullptr. Add a non-empty\n> \n> memcpy() with a nullptr parameter, which is then identified by the\n> address sanity checker.\n> \n> > data_ vector check to avoid it.\n> > \n> > The issue is noticed by running a test manually, testing the vimc\n> \n> Is it not noticed by running the unit tests with 'ninja test'?\n> \n> If not - why not - why does it have to be run manually?\n> \n> > IPA code paths in isolated mode. It is only noticed when the test\n> > is compiled with -Db_sanitize=address,undefined meson built-in option.\n> > \n> > ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null\n> > \n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > ---\n> >  src/libcamera/ipc_pipe.cpp | 7 +++++--\n> >  1 file changed, 5 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n> > index 28e20e03..c8761320 100644\n> > --- a/src/libcamera/ipc_pipe.cpp\n> > +++ b/src/libcamera/ipc_pipe.cpp\n> > @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const\n> >  \n> >  \tmemcpy(payload.data.data(), &header_, sizeof(Header));\n> >  \n> > -\t/* \\todo Make this work without copy */\n> > -\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> > +\tif (data_.size() > 0) {\n> > +\t\t/* \\todo Make this work without copy */\n> > +\t\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n\nI'd wrap this as\n\n\t\tmemcpy(payload.data.data() + sizeof(Header),\n\t\t       data_.data(), data_.size());\n\n> > +\t}\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > +\n> >  \tpayload.fds = fds_;\n> >  \n> >  \treturn payload;\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 43ABFBD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 16:54:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DD91688A5;\n\tWed, 18 Aug 2021 18:54:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74E666888A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 18:54:13 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8326EE;\n\tWed, 18 Aug 2021 18:54:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G+2uG+HO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629305653;\n\tbh=UjkQ1Gew09NGcDNnsEvzn23nCWeVaLl7cGTfJLNUVvc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G+2uG+HObIPPTUtPenJ7HwHhgNCkr7STsmhnVZSVkrl22UXLv2Dy+bb3P62Sdzfbr\n\tUHnp84w1DoyQRVVBoYKEZMqDMnx/4EQr0IhZzt2I7r8SAPA9QKQKjfPvLNtoqlfACE\n\tyzw/aO5ROBSJcqqpvDxuKdjiIyhf0L2V+3IFFAOs=","Date":"Wed, 18 Aug 2021 19:54:04 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<YR07LNT57V3WTjYQ@pendragon.ideasonboard.com>","References":"<20210818083842.31778-1-umang.jain@ideasonboard.com>\n\t<20210818083842.31778-3-umang.jain@ideasonboard.com>\n\t<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<b67cbcf8-3f56-6282-33cc-37e8d6d1a676@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18943,"web_url":"https://patchwork.libcamera.org/comment/18943/","msgid":"<20210819074606.GO1733965@pyrite.rasen.tech>","date":"2021-08-19T07:46:06","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Aug 18, 2021 at 02:08:42PM +0530, Umang Jain wrote:\n> IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket\n> payload. However, if IPCMessage is constructor with one of the\n> following constructors -\n> \n> \tIPCMessage::IPCMessage(),\n>         IPCMessage::IPCMessage(uint32_t cmd)\n>         IPCMessage::IPCMessage(const Header &header)\n> \n> The data_ vector of IPCMessage is empty and uninitialised. In that\n> case, IPCMessage::payload will try to memcpy() empty data_ vector\n> which can lead to invoking memcpy() with nullptr. Add a non-empty\n> data_ vector check to avoid it.\n> \n> The issue is noticed by running a test manually, testing the vimc\n> IPA code paths in isolated mode. It is only noticed when the test\n> is compiled with -Db_sanitize=address,undefined meson built-in option.\n> \n> ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  src/libcamera/ipc_pipe.cpp | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n> index 28e20e03..c8761320 100644\n> --- a/src/libcamera/ipc_pipe.cpp\n> +++ b/src/libcamera/ipc_pipe.cpp\n> @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const\n>  \n>  \tmemcpy(payload.data.data(), &header_, sizeof(Header));\n>  \n> -\t/* \\todo Make this work without copy */\n> -\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> +\tif (data_.size() > 0) {\n> +\t\t/* \\todo Make this work without copy */\n> +\t\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> +\t}\n> +\n>  \tpayload.fds = fds_;\n>  \n>  \treturn payload;\n> -- \n> 2.31.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3138CBD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 19 Aug 2021 07:46:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98A4368895;\n\tThu, 19 Aug 2021 09:46:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 609726888E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 19 Aug 2021 09:46:14 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC6E82A8;\n\tThu, 19 Aug 2021 09:46:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qeAehvIm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629359174;\n\tbh=dRFj+MhL/hvrgEPImkKeUL4GJ3jEV/Xk2wz4y3OiFWc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qeAehvImsnYJeLlWL4tJtt+lJCgmKRxA8e/TF27zMa0sjvT+FZWKDi4BSu/VkWmFs\n\tTUpYT0walaSODP8u4greA1rbFcR7xSrTfJhCevCJNDdbur2qlUw2+EH8vUp6EG6eHt\n\tLiz25P1oMg94KW7B+3VPj7p8a4l+DSipIuDrb8aw=","Date":"Thu, 19 Aug 2021 16:46:06 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<20210819074606.GO1733965@pyrite.rasen.tech>","References":"<20210818083842.31778-1-umang.jain@ideasonboard.com>\n\t<20210818083842.31778-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210818083842.31778-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipc_pipe: Do not run\n\tmemcpy with null arguments","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]