[{"id":26550,"web_url":"https://patchwork.libcamera.org/comment/26550/","msgid":"<20230305204810.GA20909@pendragon.ideasonboard.com>","date":"2023-03-05T20:48:10","subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Suhrid,\n\nThank you for the patch.\n\nOn Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via libcamera-devel wrote:\n> - In cases where ConverterFactoryBase::create returns a nullptr,\n> converter_->isValid() causes a segmentation fault.\n> \n> - Solve this by checking if converter_ is a nullptr.\n> \n> - Additionally, check for converter validity in the create function itself\n> and return a nullptr if the converter is invalid.\n\nThe commit message can be improved by avoiding the bullet list:\n\nThe ConverterFactoryBase::create() function returns a nullptr when no\nconverter is found. The only caller, SimpleCameraData::init(), checks if\nthe converter is valid with isValid(), but doesn't check if the pointer\nis null, which can lead to a crash.\n\nWe could check both pointer validity and converter validity in the\ncaller, but to limit the complexity in callers, it is better to check\nthe converter validity in the create() function and return a null\npointer when no valid converter is found.\n\n> Signed-off-by: Suhrid Subramaniam \n> ---\n> src/libcamera/converter.cpp | 3 ++-\n> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 3de39cff..38141da6 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -226,8 +226,9 @@ std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n> \t\t\t<< \"Creating converter from \"\n> \t\t\t<< factory->name_ << \" factory with \"\n> \t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n> +\t\tstd::unique_ptr converter_ = factory->createInstance(media);\n\nThe variable should be named 'converter' as it's a local variable. We\nuse trailing underscores for class members only.\n\n> \n> -\t\treturn factory->createInstance(media);\n> +\t\treturn converter_->isValid() ? std::move(converter_) : nullptr;\n\nThe std::move() can prevent return value optimization. It would be best\nto write\n\n\t\tif (!converter_->isValid())\n\t\t\treturn nullptr;\n\n\t\treturn converter_;\n\nWe could even keep iterating over the other factories:\n\n\t\tif (converter_->isValid())\n\t\t\treturn converter;\n\nIt may not be very useful at this point, but it wouldn't hurt and could\npossibly help later.\n\nIf you're fine with these changes, there's no need to submit a new\nversion, I can apply them locally.\n\n> \t}\n> \n> \treturn nullptr;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 24ded4db..2423ec10 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> \tMediaDevice *converter = pipe->converter();\n> \tif (converter) {\n> \t\tconverter_ = ConverterFactoryBase::create(converter);\n> -\t\tif (!converter_->isValid()) {\n> +\t\tif (!converter_) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> \t\t\tconverter_.reset();","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 E880EBDE17\n\tfor ;\n\tSun, 5 Mar 2023 20:48:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 561AB6267A;\n\tSun, 5 Mar 2023 21:48:11 +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 C473761EDE\n\tfor ;\n\tSun, 5 Mar 2023 21:48:09 +0100 (CET)","from pendragon.ideasonboard.com (85-76-103-95-nat.elisa-mobile.fi\n\t[85.76.103.95])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A4EA10C;\n\tSun, 5 Mar 2023 21:48:08 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678049291;\n\tbh=+QVeFK1JrCduCkNHok1MHvSJPry+dJ3liFO4Es4YSqw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=X1Noihdnx8QU+X7fb2aKkFo+D/mgPIQWWiQOOeZt+pJUfPIgR/zdJEqvRrQL5lIb1\n\tLN4z7xaMPheLGNa8yJOSWbbRx/uyeUQAOny0DdRnQzAPrdPp1ZGFoufR4N6sCQQqsj\n\thXXQT1nrhCNohA2Fl3nDxGI94KSHsDC0Jr2JmloepQ34maHFfL14/iEBZJxGcBDF7g\n\tfqNyrIyYzAkY+H6SRtHoWF13GA/WxE47U9Y+iEBbiFyLwPRgxAeJmciKpdFk9j4/4d\n\t8/rVLooNsYc3+JPWi04Y54fyTjsA54sbwCd76PBLCXVNZMS7OiO90i3OWpVmzPnvXL\n\t4ylCQ53L5J8lQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678049289;\n\tbh=+QVeFK1JrCduCkNHok1MHvSJPry+dJ3liFO4Es4YSqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hcBHUxipQEsA3wSLB9JOnH9+PgYKoIP/t96IKbKicWEGLpCGHC0afFilQlPsbv3cB\n\tNnLvzBhzzrbpOVUm8A7RnP5YbxD792TVc1fOHy+yWJB238ETZzYN+BeC3Ryhgj8+Sv\n\t+NjgUqlz8NDxeAs22xvjnwcHLhVnCj4JCmLRkbHs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hcBHUxip\"; dkim-atps=neutral","Date":"Sun, 5 Mar 2023 22:48:10 +0200","To":"Suhrid Subramaniam ","Message-ID":"<20230305204810.GA20909@pendragon.ideasonboard.com>","References":"<20230302190629.12506-1-suhrid.subramaniam@mediatek.com>\n\t<20230302190629.12506-2-suhrid.subramaniam@mediatek.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230302190629.12506-2-suhrid.subramaniam@mediatek.com>","Subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Suhrid Subramaniam ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26551,"web_url":"https://patchwork.libcamera.org/comment/26551/","msgid":"","date":"2023-03-05T22:14:33","subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Laurent,\n\nYes, these changes do make sense and look good to me : )\n\nThanks for the explanations too! They're very useful to me.","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 85E2ABE080\n\tfor ;\n\tSun, 5 Mar 2023 22:14:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16CC06267A;\n\tSun, 5 Mar 2023 23:14:58 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678054498;\n\tbh=11DBarMPr+EGfpTmWOcbqqktxPXcj2/SjH4Iri8+sL4=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=WvoWo8SP4VsIDUtFm85smc3uNT9cwqYJVz5o+lc08BhRKyWAUyjyIIyxpz+83akm0\n\tqS4MIK/O5hh0NmMb/KPOddT0H+d/Rw3+Vi4ILgbPKEE0UwZYUDkudRkDcLNPSs61sI\n\t4RibOyDxazfSOT0d6I33E3rJhY34Gl35xTTbbRgf8auevShKRwKfZNKJwEORWZXrPI\n\tnKYCHNwArrLpcu/RPxIeyXWUYrn5DJp+AOl+jM9ojkQ9dvFfas1Sgnkg1hoGw/IWrw\n\tLTHZKMqEkqd+RKg7/iQFJE0C2k5l3GZh2q2q5p2CiudiE4Kq53L+QvxYQfPzo0V3yo\n\tuGgxLGUWx/ccg==","To":"Laurent Pinchart , Suhrid Subramaniam\n\t","Date":"Sun, 5 Mar 2023 22:14:33 +0000","References":"<20230302190629.12506-1-suhrid.subramaniam@mediatek.com>\n\t<20230302190629.12506-2-suhrid.subramaniam@mediatek.com>\n\t<20230305204810.GA20909@pendragon.ideasonboard.com>","In-Reply-To":"<20230305204810.GA20909@pendragon.ideasonboard.com>","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Suhrid Subramaniam via libcamera-devel\n\t","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Suhrid Subramaniam ","List-Help":"","Subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26552,"web_url":"https://patchwork.libcamera.org/comment/26552/","msgid":"","date":"2023-03-05T22:27:41","subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","submitter":{"id":152,"url":"https://patchwork.libcamera.org/api/people/152/","name":"Suhrid Subramaniam","email":"suhrid.subramaniam@mediatek.com"},"content":"Hi Laurent,\n\nYes, these changes do make sense and look good to me : )\n\nThanks for the explanations too! They're very useful to me.\n\n> -----Original Message-----\n> From: Laurent Pinchart \n> Sent: Sunday, March 5, 2023 12:48 PM\n> To: Suhrid Subramaniam \n> Cc: libcamera-devel@lists.libcamera.org; Suhrid Subramaniam\n> \n> Subject: Re: [libcamera-devel] [libcamera-devel, v3, 1/1] libcamera: converter:\n> Check converter validity\n> \n> Hi Suhrid,\n> \n> Thank you for the patch.\n> \n> On Thu, Mar 02, 2023 at 11:06:29AM -0800, Suhrid Subramaniam via\n> libcamera-devel wrote:\n> > - In cases where ConverterFactoryBase::create returns a nullptr,\n> > converter_->isValid() causes a segmentation fault.\n> >\n> > - Solve this by checking if converter_ is a nullptr.\n> >\n> > - Additionally, check for converter validity in the create function itself\n> > and return a nullptr if the converter is invalid.\n> \n> The commit message can be improved by avoiding the bullet list:\n> \n> The ConverterFactoryBase::create() function returns a nullptr when no\n> converter is found. The only caller, SimpleCameraData::init(), checks if the\n> converter is valid with isValid(), but doesn't check if the pointer is null,\n> which can lead to a crash.\n> \n> We could check both pointer validity and converter validity in the caller, but\n> to limit the complexity in callers, it is better to check the converter validity\n> in the create() function and return a null pointer when no valid converter is\n> found.\n> \n> > Signed-off-by: Suhrid Subramaniam \n> > ---\n> > src/libcamera/converter.cpp | 3 ++-\n> > src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> > 2 files changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > index 3de39cff..38141da6 100644\n> > --- a/src/libcamera/converter.cpp\n> > +++ b/src/libcamera/converter.cpp\n> > @@ -226,8 +226,9 @@ std::unique_ptr\n> ConverterFactoryBase::create(MediaDevice *media)\n> > \t\t\t<< \"Creating converter from \"\n> > \t\t\t<< factory->name_ << \" factory with \"\n> > \t\t\t<< (it == compatibles.end() ? \"no\" : media->driver())\n> << \"\n> > alias.\";\n> > +\t\tstd::unique_ptr converter_ =\n> > +factory->createInstance(media);\n> \n> The variable should be named 'converter' as it's a local variable. We use\n> trailing underscores for class members only.\n> \n> >\n> > -\t\treturn factory->createInstance(media);\n> > +\t\treturn converter_->isValid() ? std::move(converter_) :\n> nullptr;\n> \n> The std::move() can prevent return value optimization. It would be best to\n> write\n> \n> \t\tif (!converter_->isValid())\n> \t\t\treturn nullptr;\n> \n> \t\treturn converter_;\n> \n> We could even keep iterating over the other factories:\n> \n> \t\tif (converter_->isValid())\n> \t\t\treturn converter;\n> \n> It may not be very useful at this point, but it wouldn't hurt and could\n> possibly help later.\n> \n> If you're fine with these changes, there's no need to submit a new version, I\n> can apply them locally.\n> \n> > \t}\n> >\n> > \treturn nullptr;\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp\n> > b/src/libcamera/pipeline/simple/simple.cpp\n> > index 24ded4db..2423ec10 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -493,7 +493,7 @@ int SimpleCameraData::init()\n> > \tMediaDevice *converter = pipe->converter();\n> > \tif (converter) {\n> > \t\tconverter_ = ConverterFactoryBase::create(converter);\n> > -\t\tif (!converter_->isValid()) {\n> > +\t\tif (!converter_) {\n> > \t\t\tLOG(SimplePipeline, Warning)\n> > \t\t\t\t<< \"Failed to create converter, disabling\n> format conversion\";\n> > \t\t\tconverter_.reset();\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 9C57CBDE17\n\tfor ;\n\tSun, 5 Mar 2023 22:28:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79F7B6267A;\n\tSun, 5 Mar 2023 23:27:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678055279;\n\tbh=m/uzcuwtf9CJC44eHTDwt0c/4IDye5U5Meer1V8BCRY=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=dXAxrTXuUC8piwco1sgY1DUc58GYjVbeWHP+SiCDL+WGliHcoaQIdTkNbqHtN1YCe\n\tX60Kkj/al/f56636XK1E9UR6QVhuXp/Kmhm9azcCEOOJH4Y0zobYgtpXcmVpSDosT4\n\tvPQ+33PiIWguew4HQ7bPnH57ZF1yu/+su/kshsU2hixTGveOPXyTtrDQiMWrN+lT7U\n\tUtH6ajU1zJE4sKHWz8A2u7z8PdUuyN1USQVmHoPBS4vHPUmvjRAAXIRuQ+kxz90MQX\n\tsMxI1zXYOVSovcDeMODoonSMHQvC5J8H4GaaPaoY9d1MQ+n536KGcNxxCP8Fgc9vyn\n\tlcBWovmf31x1A==","To":"Laurent Pinchart , Suhrid Subramaniam\n\t","Date":"Sun, 5 Mar 2023 22:27:41 +0000","References":"<20230302190629.12506-1-suhrid.subramaniam@mediatek.com>\n\t<20230302190629.12506-2-suhrid.subramaniam@mediatek.com>\n\t<20230305204810.GA20909@pendragon.ideasonboard.com>","In-Reply-To":"<20230305204810.GA20909@pendragon.ideasonboard.com>","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Suhrid Subramaniam via libcamera-devel\n\t","Precedence":"list","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Suhrid Subramaniam ","List-Help":"","Subject":"Re: [libcamera-devel] [libcamera-devel, v3,\n\t1/1] libcamera: converter: Check converter validity","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]