[{"id":32808,"web_url":"https://patchwork.libcamera.org/comment/32808/","msgid":"<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>","date":"2024-12-17T10:42:50","subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Antoine Bouyer (2024-12-17 09:51:37)\n> This patch fixes below crash, with styhead's gcc compiler (version 14.2.0),\n> which occurs when optional colorSpace parameter is not filled.\n\nI find https://cbea.ms/git-commit/ a good reference for writing clear\ncommit messages.\n\nWe prefer not to say things like \"this patch fixes\", but instead explain\nthe problem and the solution.\n\nIn this case, the problem is that the KMS sink does not correctly\ncheck the optional colorSpace parameter of the stream configuration\nbefore dereferencing it.\n\nThe solution is to ensure that the std::optional colorSpace has a value\nbefore accessing it.\n\nAnd explaining that it was identified with the specific toolcahin and\nenvironement is all helpful additional information that could follow.\n\nI would also recommend shortening the subject line. The 50,72 chars\nmentioned in https://cbea.ms/git-commit/ is a good target, but not\nalways achieveable especially with the tags in front. But it makes a big\ndifference to the readability of the commit logs, and also the subject\nhere becomes part of the release notes.\n\n\"\"\"\napps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior\n\"\"\"\n\ncould be something shorter like:\n\n\"\"\"\napps: cam: kms_sink: Verify colorSpace definition before dereference\n\"\"\"\n\n\n> \n> /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48\n> 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp\n> ace; _Dp = std::_Optional_base]: Assertion 'this->_M_is_engaged()' fai\n> led.\n> Aborted (core dumped)\n> \n> As detailed in the \"operator->\" page:\n> (https://en.cppreference.com/w/cpp/utility/optional/operator*)\n> \n> \"This operator does not check whether the optional contains a value!\"\n> \n> Use has_value() as a fix to make sure this property exists and prevent crash.\n> \n\nLets add this tag here too:\n\nFixes: 6d7ddace5278 (\"cam: Add KMS sink class\")\n\n> Signed-off-by: Antoine Bouyer \n> ---\n> src/apps/cam/kms_sink.cpp | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> index 672c985..8f3b867 100644\n> --- a/src/apps/cam/kms_sink.cpp\n> +++ b/src/apps/cam/kms_sink.cpp\n> @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> colorEncoding_ = std::nullopt;\n> colorRange_ = std::nullopt;\n> \n> - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> + if (!cfg.colorSpace.has_value() ||\n> + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n\ncfg.colorSpace is indeed a std::optional so this makes sense.\n\nI'm surprised this hasn't been hit before ?\n\nReviewed-by: Kieran Bingham \n\n> return 0;\n> \n> /*\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","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 0B37DC32F6\n\tfor ;\n\tTue, 17 Dec 2024 10:42:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1B9E67FC7;\n\tTue, 17 Dec 2024 11:42:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F16F67FC0\n\tfor ;\n\tTue, 17 Dec 2024 11:42:53 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 642C33E;\n\tTue, 17 Dec 2024 11:42:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Pl4Bosrg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734432136;\n\tbh=axO0zvfaLlrDq3oy9wHT0P+8HQdh5dzOTyNteljrxec=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Pl4BosrglLBPZFj4qwO0/2dhGZaITeyJJjS0qej/sdy8+6+rIWNi4KmT7MqJp5Xwr\n\tmuNDh9SLzCUrGLTcq8mNDO/OAQQh+l5RBdG41H+8J26faq8xPKIwuEauKHtGs6PCWq\n\t8jr629CW/zDDW8NL5ByraG17RiHXgrzQQqOi8idE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241217095137.132370-1-antoine.bouyer@nxp.com>","References":"<20241217095137.132370-1-antoine.bouyer@nxp.com>","Subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","From":"Kieran Bingham ","Cc":"julien.vuillaumier@nxp.com, antoine.bouyer@nxp.com","To":"Antoine Bouyer ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 17 Dec 2024 10:42:50 +0000","Message-ID":"<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32812,"web_url":"https://patchwork.libcamera.org/comment/32812/","msgid":"<20241217120840.GE2025@pendragon.ideasonboard.com>","date":"2024-12-17T12:08:40","subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote:\n> Quoting Antoine Bouyer (2024-12-17 09:51:37)\n> > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0),\n> > which occurs when optional colorSpace parameter is not filled.\n> \n> I find https://cbea.ms/git-commit/ a good reference for writing clear\n> commit messages.\n> \n> We prefer not to say things like \"this patch fixes\", but instead explain\n> the problem and the solution.\n> \n> In this case, the problem is that the KMS sink does not correctly\n> check the optional colorSpace parameter of the stream configuration\n> before dereferencing it.\n> \n> The solution is to ensure that the std::optional colorSpace has a value\n> before accessing it.\n> \n> And explaining that it was identified with the specific toolcahin and\n> environement is all helpful additional information that could follow.\n> \n> I would also recommend shortening the subject line. The 50,72 chars\n> mentioned in https://cbea.ms/git-commit/ is a good target, but not\n> always achieveable especially with the tags in front. But it makes a big\n> difference to the readability of the commit logs, and also the subject\n> here becomes part of the release notes.\n> \n> \"\"\"\n> apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior\n> \"\"\"\n> \n> could be something shorter like:\n> \n> \"\"\"\n> apps: cam: kms_sink: Verify colorSpace definition before dereference\n> \"\"\"\n\nMaybe s/definition/presence/\n\n> > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48\n> > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp\n> > ace; _Dp = std::_Optional_base]: Assertion 'this->_M_is_engaged()' fai\n> > led.\n> > Aborted (core dumped)\n> > \n> > As detailed in the \"operator->\" page:\n> > (https://en.cppreference.com/w/cpp/utility/optional/operator*)\n> > \n> > \"This operator does not check whether the optional contains a value!\"\n> > \n> > Use has_value() as a fix to make sure this property exists and prevent crash.\n> > \n> \n> Lets add this tag here too:\n> \n> Fixes: 6d7ddace5278 (\"cam: Add KMS sink class\")\n> \n> > Signed-off-by: Antoine Bouyer \n> > ---\n> > src/apps/cam/kms_sink.cpp | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> > index 672c985..8f3b867 100644\n> > --- a/src/apps/cam/kms_sink.cpp\n> > +++ b/src/apps/cam/kms_sink.cpp\n> > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > colorEncoding_ = std::nullopt;\n> > colorRange_ = std::nullopt;\n> > \n> > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> > + if (!cfg.colorSpace.has_value() ||\n> > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> \n> cfg.colorSpace is indeed a std::optional so this makes sense.\n> \n> I'm surprised this hasn't been hit before ?\n\nThe member is optional primarily to allow applications to get the\ndefault color space. We also allow pipeline handlers to leave it unset\nwhen they don't know what color space is produced, but that shouldn't be\nthe rule. Actually, except for the simple pipeline handler (and possibly\nfor UVC, I haven't checked), I would make it mandatory for pipeline\nhandlers to return a valid color space. We could also require them to\nreport ColorSpace::Raw when they don't know.\n\n> Reviewed-by: Kieran Bingham \n> \n> > return 0;\n> > \n> > /*","headers":{"Return-Path":"","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 40AC9BD1F1\n\tfor ;\n\tTue, 17 Dec 2024 12:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 949AE67FCB;\n\tTue, 17 Dec 2024 13:08:44 +0100 (CET)","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 BDD3E67F0D\n\tfor ;\n\tTue, 17 Dec 2024 13:08:42 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3163E3E;\n\tTue, 17 Dec 2024 13:08:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fvYUQsfH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734437285;\n\tbh=xfHkQaq+aF7hLHbmRq2CjEdEfI5L6QgHXuqNp6Awfi0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fvYUQsfHiWJRGZOtFtFMABT+fHNv2gBWRK35s/GeTFCya7tXNCdi5XMNU3bUNPTkM\n\tlrqQa/LGGd+bSiS/evC2j9Otrl1alx8oOBnsilgWtluU1VGQoQXo3aFnuw3HRO02ex\n\tKvdtUemj1SuISXCHsxJ0AxCo3Wm8Q8q2PcWKf5qg=","Date":"Tue, 17 Dec 2024 14:08:40 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Antoine Bouyer ,\n\tlibcamera-devel@lists.libcamera.org, julien.vuillaumier@nxp.com","Subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","Message-ID":"<20241217120840.GE2025@pendragon.ideasonboard.com>","References":"<20241217095137.132370-1-antoine.bouyer@nxp.com>\n\t<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34680,"web_url":"https://patchwork.libcamera.org/comment/34680/","msgid":"<175101248000.3281735.6503820125870604066@ping.linuxembedded.co.uk>","date":"2025-06-27T08:21:20","subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-12-17 12:08:40)\n> On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote:\n> > Quoting Antoine Bouyer (2024-12-17 09:51:37)\n> > > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0),\n> > > which occurs when optional colorSpace parameter is not filled.\n\nAntoine, did you fix this in your pipeline handler in the end?\n\nIs this commit still applicable otherwise?\n\nPerhaps what we actually need is a check in lc-compliance to make sure\nthe colorSpace is always defined by pipeline handlers.\n\nUltimately I'd like to know how to close this patch - will we have a v2\n- or do we reject this and instead supply a validation in the compliance\ntests?\n\n--\nKieran\n\n\n> > \n> > I find https://cbea.ms/git-commit/ a good reference for writing clear\n> > commit messages.\n> > \n> > We prefer not to say things like \"this patch fixes\", but instead explain\n> > the problem and the solution.\n> > \n> > In this case, the problem is that the KMS sink does not correctly\n> > check the optional colorSpace parameter of the stream configuration\n> > before dereferencing it.\n> > \n> > The solution is to ensure that the std::optional colorSpace has a value\n> > before accessing it.\n> > \n> > And explaining that it was identified with the specific toolcahin and\n> > environement is all helpful additional information that could follow.\n> > \n> > I would also recommend shortening the subject line. The 50,72 chars\n> > mentioned in https://cbea.ms/git-commit/ is a good target, but not\n> > always achieveable especially with the tags in front. But it makes a big\n> > difference to the readability of the commit logs, and also the subject\n> > here becomes part of the release notes.\n> > \n> > \"\"\"\n> > apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior\n> > \"\"\"\n> > \n> > could be something shorter like:\n> > \n> > \"\"\"\n> > apps: cam: kms_sink: Verify colorSpace definition before dereference\n> > \"\"\"\n> \n> Maybe s/definition/presence/\n> \n> > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48\n> > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp\n> > > ace; _Dp = std::_Optional_base]: Assertion 'this->_M_is_engaged()' fai\n> > > led.\n> > > Aborted (core dumped)\n> > > \n> > > As detailed in the \"operator->\" page:\n> > > (https://en.cppreference.com/w/cpp/utility/optional/operator*)\n> > > \n> > > \"This operator does not check whether the optional contains a value!\"\n> > > \n> > > Use has_value() as a fix to make sure this property exists and prevent crash.\n> > > \n> > \n> > Lets add this tag here too:\n> > \n> > Fixes: 6d7ddace5278 (\"cam: Add KMS sink class\")\n> > \n> > > Signed-off-by: Antoine Bouyer \n> > > ---\n> > > src/apps/cam/kms_sink.cpp | 3 ++-\n> > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> > > index 672c985..8f3b867 100644\n> > > --- a/src/apps/cam/kms_sink.cpp\n> > > +++ b/src/apps/cam/kms_sink.cpp\n> > > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > colorEncoding_ = std::nullopt;\n> > > colorRange_ = std::nullopt;\n> > > \n> > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> > > + if (!cfg.colorSpace.has_value() ||\n> > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> > \n> > cfg.colorSpace is indeed a std::optional so this makes sense.\n> > \n> > I'm surprised this hasn't been hit before ?\n> \n> The member is optional primarily to allow applications to get the\n> default color space. We also allow pipeline handlers to leave it unset\n> when they don't know what color space is produced, but that shouldn't be\n> the rule. Actually, except for the simple pipeline handler (and possibly\n> for UVC, I haven't checked), I would make it mandatory for pipeline\n> handlers to return a valid color space. We could also require them to\n> report ColorSpace::Raw when they don't know.\n> \n> > Reviewed-by: Kieran Bingham \n> > \n> > > return 0;\n> > > \n> > > /*\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","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 F1675C3237\n\tfor ;\n\tFri, 27 Jun 2025 08:21:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6EA5668DF7;\n\tFri, 27 Jun 2025 10:21:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D90562C43\n\tfor ;\n\tFri, 27 Jun 2025 10:21:23 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E3066A8;\n\tFri, 27 Jun 2025 10:21:03 +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=\"JJ+DD3c8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751012463;\n\tbh=XUJzfAV5qQBbu3sfHaB+T2iP3Pgws09i5w3YFzdzdb0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JJ+DD3c8OCOBqD/NRs4udcCTvZjZx0ckC26oBlgVuk+FNgNp5zbyqr7yNJ7T0/35g\n\t1RNN+Deim3Z1mdZdnrh6JUtPpTtkeee9BMXrBndvO+fN4sOozbO59qkCe6MCs9/4iO\n\tAxPGi5z0M6ph9IfxiA/viUtXw6E8tathfo3AA+vQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241217120840.GE2025@pendragon.ideasonboard.com>","References":"<20241217095137.132370-1-antoine.bouyer@nxp.com>\n\t<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>\n\t<20241217120840.GE2025@pendragon.ideasonboard.com>","Subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","From":"Kieran Bingham ","Cc":"Antoine Bouyer ,\n\tlibcamera-devel@lists.libcamera.org, julien.vuillaumier@nxp.com","To":"Laurent Pinchart ","Date":"Fri, 27 Jun 2025 09:21:20 +0100","Message-ID":"<175101248000.3281735.6503820125870604066@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34681,"web_url":"https://patchwork.libcamera.org/comment/34681/","msgid":"<175101252765.3281735.17687361015082617262@ping.linuxembedded.co.uk>","date":"2025-06-27T08:22:07","subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-06-27 09:21:20)\n> Quoting Laurent Pinchart (2024-12-17 12:08:40)\n> > On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote:\n> > > Quoting Antoine Bouyer (2024-12-17 09:51:37)\n> > > > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0),\n> > > > which occurs when optional colorSpace parameter is not filled.\n> \n> Antoine, did you fix this in your pipeline handler in the end?\n> \n> Is this commit still applicable otherwise?\n> \n> Perhaps what we actually need is a check in lc-compliance to make sure\n> the colorSpace is always defined by pipeline handlers.\n> \n> Ultimately I'd like to know how to close this patch - will we have a v2\n> - or do we reject this and instead supply a validation in the compliance\n> tests?\n> \n\nAnd now I work up through my mails I instantly see a v2.\nSorry for the noise - I'll just mark this as superceeded.\n\n> --\n> Kieran\n> \n> \n> > > \n> > > I find https://cbea.ms/git-commit/ a good reference for writing clear\n> > > commit messages.\n> > > \n> > > We prefer not to say things like \"this patch fixes\", but instead explain\n> > > the problem and the solution.\n> > > \n> > > In this case, the problem is that the KMS sink does not correctly\n> > > check the optional colorSpace parameter of the stream configuration\n> > > before dereferencing it.\n> > > \n> > > The solution is to ensure that the std::optional colorSpace has a value\n> > > before accessing it.\n> > > \n> > > And explaining that it was identified with the specific toolcahin and\n> > > environement is all helpful additional information that could follow.\n> > > \n> > > I would also recommend shortening the subject line. The 50,72 chars\n> > > mentioned in https://cbea.ms/git-commit/ is a good target, but not\n> > > always achieveable especially with the tags in front. But it makes a big\n> > > difference to the readability of the commit logs, and also the subject\n> > > here becomes part of the release notes.\n> > > \n> > > \"\"\"\n> > > apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior\n> > > \"\"\"\n> > > \n> > > could be something shorter like:\n> > > \n> > > \"\"\"\n> > > apps: cam: kms_sink: Verify colorSpace definition before dereference\n> > > \"\"\"\n> > \n> > Maybe s/definition/presence/\n> > \n> > > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48\n> > > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp\n> > > > ace; _Dp = std::_Optional_base]: Assertion 'this->_M_is_engaged()' fai\n> > > > led.\n> > > > Aborted (core dumped)\n> > > > \n> > > > As detailed in the \"operator->\" page:\n> > > > (https://en.cppreference.com/w/cpp/utility/optional/operator*)\n> > > > \n> > > > \"This operator does not check whether the optional contains a value!\"\n> > > > \n> > > > Use has_value() as a fix to make sure this property exists and prevent crash.\n> > > > \n> > > \n> > > Lets add this tag here too:\n> > > \n> > > Fixes: 6d7ddace5278 (\"cam: Add KMS sink class\")\n> > > \n> > > > Signed-off-by: Antoine Bouyer \n> > > > ---\n> > > > src/apps/cam/kms_sink.cpp | 3 ++-\n> > > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> > > > index 672c985..8f3b867 100644\n> > > > --- a/src/apps/cam/kms_sink.cpp\n> > > > +++ b/src/apps/cam/kms_sink.cpp\n> > > > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > > > colorEncoding_ = std::nullopt;\n> > > > colorRange_ = std::nullopt;\n> > > > \n> > > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> > > > + if (!cfg.colorSpace.has_value() ||\n> > > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)\n> > > \n> > > cfg.colorSpace is indeed a std::optional so this makes sense.\n> > > \n> > > I'm surprised this hasn't been hit before ?\n> > \n> > The member is optional primarily to allow applications to get the\n> > default color space. We also allow pipeline handlers to leave it unset\n> > when they don't know what color space is produced, but that shouldn't be\n> > the rule. Actually, except for the simple pipeline handler (and possibly\n> > for UVC, I haven't checked), I would make it mandatory for pipeline\n> > handlers to return a valid color space. We could also require them to\n> > report ColorSpace::Raw when they don't know.\n> > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > return 0;\n> > > > \n> > > > /*\n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart","headers":{"Return-Path":"","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 206A8C3237\n\tfor ;\n\tFri, 27 Jun 2025 08:22:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B99AE68DF7;\n\tFri, 27 Jun 2025 10:22:12 +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 C17C262C43\n\tfor ;\n\tFri, 27 Jun 2025 10:22:10 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6798B6A8;\n\tFri, 27 Jun 2025 10:21:51 +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=\"A5q14pn2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751012511;\n\tbh=jZCMZf3LX1RSm1nPXRiZw8ssLimjFgTgK306ocuCujM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=A5q14pn2YvgfQdZ0GtfL334eZ8C//9QyDOlISLDgmJvG+z3e0bQ8l9PVhGvDISinm\n\tjaaDfq7+ZOeCcxQ11sJ4wqHZML+NATjYFVyvkuipYugM3dAgYCxviMAeuO/RhiS57X\n\tSn8lIyBZTR0WUCnm8ZMd3DISYBAAQk8nmGLK0/7c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175101248000.3281735.6503820125870604066@ping.linuxembedded.co.uk>","References":"<20241217095137.132370-1-antoine.bouyer@nxp.com>\n\t<173443217046.543771.9912537831281751048@ping.linuxembedded.co.uk>\n\t<20241217120840.GE2025@pendragon.ideasonboard.com>\n\t<175101248000.3281735.6503820125870604066@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH] apps: cam: kms_sink: Verify colorSpace is defined before\n\tdereferencing to avoid undefined behavior","From":"Kieran Bingham ","Cc":"Antoine Bouyer ,\n\tlibcamera-devel@lists.libcamera.org, julien.vuillaumier@nxp.com","To":"Laurent Pinchart ","Date":"Fri, 27 Jun 2025 09:22:07 +0100","Message-ID":"<175101252765.3281735.17687361015082617262@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]