[{"id":25083,"web_url":"https://patchwork.libcamera.org/comment/25083/","msgid":"<166384583742.56880.2887323021444443507@Monstersaurus>","date":"2022-09-22T11:23:57","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: controls: validate all\n\tControlInfo values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Christian Rauch via libcamera-devel (2022-09-21 21:31:48)\n> ControlInfoMap::validate only checks the 'min' type against the ControlId\n> type. Extend this with checks against the 'max' type and the 'def' type,\n> if a default is specified. This forces the min/max bounds to have the same\n> type as the controlled value, but leaves the default optional.\n> \n> Signed-off-by: Christian Rauch \n> ---\n> src/libcamera/controls.cpp | 22 ++++++++++++++++++++++\n> 1 file changed, 22 insertions(+)\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index bc3db4f6..e7251507 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -701,6 +701,28 @@ bool ControlInfoMap::validate()\n> ? ControlTypeInteger32 : id->type();\n> const ControlInfo &info = ctrl.second;\n> \n> + if (!(info.min().isArray() == info.max().isArray() &&\n> + info.min().numElements() == info.max().numElements())) {\n\nDoes numElements work even if the type is not an array? (i.e. will it\nreturn '1'?)\n\nIf so - that could perhaps be simplified to just:\n\t\tif (info.min().numElements() != info.max().numElements())\n\n\n\n> + LOG(Controls, Error)\n> + << \"Control \" << utils::hex(id->id())\n> + << \" range must have the same dimension.\";\n> + return false;\n> + }\n> +\n> + if (info.min().type() != info.max().type()) {\n\nThis will already enforce the types are the same. But perhaps the type\nchecking should be before the size checking above.\n\n> + LOG(Controls, Error)\n> + << \"Control \" << utils::hex(id->id())\n> + << \" range types mismatch\";\n> + return false;\n> + }\n> +\n> + if (info.def().type() != ControlTypeNone && (info.min().type() != info.def().type())) {\n\nPerhaps neater over two lines, and I believe the second parenthesis\nisn't required:\n\n\t\tif (info.def().type() != ControlTypeNone &&\n\t\t info.min().type() != info.def().type()) {\n\n\n\n> + LOG(Controls, Error)\n> + << \"Control \" << utils::hex(id->id())\n\nDo we have access to any human readable string here instead of a hex of\nthe id()?\n\nIf so - It would be better to use that (but it may not be available)\n\nWith those considered:\n\nReviewed-by: Kieran Bingham \n\n> + << \" default value and info type mismatch\";\n> + return false;\n> + }\n> +\n> if (info.min().type() != rangeType) {\n> LOG(Controls, Error)\n> << \"Control \" << utils::hex(id->id())\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 28C30C0DA4\n\tfor ;\n\tThu, 22 Sep 2022 11:24:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C9096220B;\n\tThu, 22 Sep 2022 13:24:02 +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 E5EDE6219A\n\tfor ;\n\tThu, 22 Sep 2022 13:24:00 +0200 (CEST)","from pendragon.ideasonboard.com\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 5AB756BE;\n\tThu, 22 Sep 2022 13:24:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663845842;\n\tbh=x3i5dVnlMqlS0ylde53owy115q8r1umaGUh4A9d7I4w=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=mpXrNnL50W7iKoKLu5rQpAGUYU7t0Wh0AMhdncQ52jAKnt3qM8kS0gGq6NHIDbnKG\n\tZ6MsxtPr9u488QBC4jTDlIhJMFOO1LGeypt7yVls6eXDGjHnkAU2zQzFvShgqhQVz2\n\t5WC+O0K2uUIeiSZsR2DwFGteMWb0qxti3mXnqp2fGQtlF8IwxhyrDNYytAqpzh2wZD\n\t34EfHXGCMFTnuEO9lJvPbZIsfhctEKfA23bBQui+h8d/cpSamUvxST5nUA4OmBcYVh\n\tdn70le1S/a23OZG+SLjgT9i76VohyHUleVcqP7pP3dtpRGhOFjndNWY9lng/39dF2a\n\tQJEOcS4FaWo1w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663845840;\n\tbh=x3i5dVnlMqlS0ylde53owy115q8r1umaGUh4A9d7I4w=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=r+7AvKL6ya5FTXAchK0zrxWlgojqFf0zPajxrwtQSGi30jvbccbcv0KadVQDluRAC\n\tV7G5TYPavJQN2hIIK5BHIWd3l+rtxPVzSp3jRj+6n4FAOo5/y4YsM3ZsWd8cQqSGnw\n\tfmLIXxj1VomFNSzTUi+J9GIAl1FoLev40/WoH/Zg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"r+7AvKL6\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220921203148.534883-1-Rauch.Christian@gmx.de>","References":"<20220921203148.534883-1-Rauch.Christian@gmx.de>","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 22 Sep 2022 12:23:57 +0100","Message-ID":"<166384583742.56880.2887323021444443507@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: controls: validate all\n\tControlInfo values","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25093,"web_url":"https://patchwork.libcamera.org/comment/25093/","msgid":"","date":"2022-09-22T20:21:59","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: controls: validate all\n\tControlInfo values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nAm 22.09.22 um 13:23 schrieb Kieran Bingham:\n> Quoting Christian Rauch via libcamera-devel (2022-09-21 21:31:48)\n>> ControlInfoMap::validate only checks the 'min' type against the ControlId\n>> type. Extend this with checks against the 'max' type and the 'def' type,\n>> if a default is specified. This forces the min/max bounds to have the same\n>> type as the controlled value, but leaves the default optional.\n>>\n>> Signed-off-by: Christian Rauch \n>> ---\n>> src/libcamera/controls.cpp | 22 ++++++++++++++++++++++\n>> 1 file changed, 22 insertions(+)\n>>\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index bc3db4f6..e7251507 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -701,6 +701,28 @@ bool ControlInfoMap::validate()\n>> ? ControlTypeInteger32 : id->type();\n>> const ControlInfo &info = ctrl.second;\n>>\n>> + if (!(info.min().isArray() == info.max().isArray() &&\n>> + info.min().numElements() == info.max().numElements())) {\n>\n> Does numElements work even if the type is not an array? (i.e. will it\n> return '1'?)\n>\n> If so - that could perhaps be simplified to just:\n> \t\tif (info.min().numElements() != info.max().numElements())\n\nnumElements() will return a value in any case. I think it will be 0 for\nscalars, but without checking isArray() it is not clear if this is a\nscalar or empty array.\n\n>\n>> + LOG(Controls, Error)\n>> + << \"Control \" << utils::hex(id->id())\n>> + << \" range must have the same dimension.\";\n>> + return false;\n>> + }\n>> +\n>> + if (info.min().type() != info.max().type()) {\n>\n> This will already enforce the types are the same. But perhaps the type\n> checking should be before the size checking above.\n\nNot sure why \"already\". I think this is the first place we check the\nmin/max types.\n\n>\n>> + LOG(Controls, Error)\n>> + << \"Control \" << utils::hex(id->id())\n>> + << \" range types mismatch\";\n>> + return false;\n>> + }\n>> +\n>> + if (info.def().type() != ControlTypeNone && (info.min().type() != info.def().type())) {\n>\n> Perhaps neater over two lines, and I believe the second parenthesis\n> isn't required:\n>\n> \t\tif (info.def().type() != ControlTypeNone &&\n> \t\t info.min().type() != info.def().type()) {\n>\n>\n>\n>> + LOG(Controls, Error)\n>> + << \"Control \" << utils::hex(id->id())\n>\n> Do we have access to any human readable string here instead of a hex of\n> the id()?\n>\n> If so - It would be better to use that (but it may not be available)\n\nI used the hex-id as in the other test, but changed this to the 'name'.\n\n>\n> With those considered:\n>\n> Reviewed-by: Kieran Bingham \n>\n>> + << \" default value and info type mismatch\";\n>> + return false;\n>> + }\n>> +\n>> if (info.min().type() != rangeType) {\n>> LOG(Controls, Error)\n>> << \"Control \" << utils::hex(id->id())\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 5260BBD16B\n\tfor ;\n\tThu, 22 Sep 2022 20:22:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADE506219A;\n\tThu, 22 Sep 2022 22:22:02 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.20])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E83716219A\n\tfor ;\n\tThu, 22 Sep 2022 22:22:00 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx104\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id\n\t1Mz9Z5-1pXI3n1YDj-00wGfe for\n\t; Thu, 22 Sep 2022 22:22:00 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663878122;\n\tbh=F4VdiPwwDhT/WJiSyc1Z+2MCezFz6FEEIgbw/wVBWCo=;\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:\n\tFrom;\n\tb=qGpQ1S/ct43ZuuGiF5QhvY2a8ysk7uz1biEeAg+a88mEkfZkZNzdlJ9avMowsda9G\n\tVJPCET/+Vi7GGQd3j1JpGzPqGIHJaPADw2ThQhsa3el9DQzOZWVkrHPyVYXQ1cKbDZ\n\tg+48SbsxbrmEUSNVxxH8kYejZUCPklvyzlNwuuzTFq6wvhRnsfejzWn6I5T6iI390+\n\t0DPAFy0VMZnx+YDyLgbYqtmsMwHETIVS6x9HtPaaJmmiZ1iul3WIxXmIxWO6fMuMen\n\tFdxnSfFcj68WsmHLs97YRB2+sM9Nge7FPDUitJiyvSz6HozbCv6To6cCwDo5cOPLSc\n\tWyKYPCaNvj4bg==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1663878120;\n\tbh=F4VdiPwwDhT/WJiSyc1Z+2MCezFz6FEEIgbw/wVBWCo=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=lLunT+gJHExhq8NM1E5OpcC1rRUAGdcRHmX7FIbwuA0TQNBW3eSrxFlbs2YZtrvku\n\tXUWFeCHwXfRvUjuhXVbSdwpGwYJhIoFbbcLVNnfa2KXbcZ4l/IL3yy+QNVV7GsYRZF\n\tESZ9Z0zupBhWoofE1CUSO3mR8OJnA2X5RV/iKIGI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"lLunT+gJ\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Thu, 22 Sep 2022 22:21:59 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220921203148.534883-1-Rauch.Christian@gmx.de>\n\t<166384583742.56880.2887323021444443507@Monstersaurus>","In-Reply-To":"<166384583742.56880.2887323021444443507@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:X4EvFjRj7fUM6NnSL8fkZQ5rGSeyz2UeCL5MMyay34s5sPAC09b\n\t2uZNe2pjxIc2klWahAC/tHGzc1TpbWQOwyGa4qRtUtmACCSGCYjKLTGV6Af5MSHJGQ/uDMC\n\ttYMM1qvEJ1RSYx2cacbrnzC0bk2Oal+XPwt1b/PYnhDnyqPpfqoWDwHI06djRhvhGjJrHEr\n\twI0inaVb++TEGTQpOzr0A==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:DiYY2xFCo+E=:bPMMoCB7zQBWe8HIrTenQw\n\toFC5ONrW/go202wrQ4TXFcjPJNv+4bwMTpSEdrVf5H4cvlxf8WhhvFC+N1W9VUOeRjPwpuAwK\n\tNBHr4XZxYgh6tPvuND7hmiwCivUcft8RAdY96k9mcUWv+naqbAT0rs+Ggq5tmmN8qK5uP2Vbi\n\t1DmH6oof+B9vLuQvgrDawJ+Y7zxgUKpb256Sx3sPLFCqPLGry8jqJBnoCEUUMuFLMTLEVdyx+\n\tj+y1uAQ+HClRbg2p1BNzhhXZXq7A3fbhzFV6KxO+9cJfA0vLpblXennlXN384Oo0Zq19+lgVV\n\tBs1dQE6OJD1lPWJHMxPXUzL3Dawg1NB+/83DGq5lve3H+z0ikMK6WyobXbujjuqK9sdM4VpGn\n\tekQjQOJr5QFRm6yQuMLnFBLAam8Kh52WCY0AFXVYQDPhbQa8K8XQargi7IigT7Z1yeCsOcAVH\n\tnPA6JsHngyxWU81gZq3WjorH0MGYBHLE+dsaK1hkqouQDG2zGZkmQ1NsyrROyqHKB8/Ffb4w1\n\tVSR5n9HQWVw6eEtBTAFKAiTr+qG3AKAQC+TgbFMyfYi3Q95tcIKvGDSBSDDRiZB8jkzrY3RJp\n\tYI0TA7Ghyf6wu7nkrdCcTBn58oExp4N1EPWIpbx6WN+LhLyzbkTwPC6NRWfyenhP1GVuNniYU\n\t1TRDJZylBe5eGH/1Dfnyx9u+uFU86AlZE6hKrZjoyHPIbEf90nKfvxnUxOfXly8PfZ4avAzfY\n\tUtKJERxppLGpbEuDVDmMI+RVxJJM6dBflStCfFT/SXJ9RbRSwIfEoPdU7NsOHdp9EfLic+6+v\n\tBQRchicz0swrFdlPKy6oCvfEqzOcqduEEsBKIhvXiDasJC2sHRQQ9txvipnRHXTAzTVi8quGu\n\tNKLJyeLTqlA24IPgjNeNKzewSxP9tEcqHUJjIuKoiXqTwCpXBPT6gGS1SHE4jz6HkrEkgSuBh\n\tWYdlzHXAVVTwRXi9hSrPXP29LJpiStlDirvf2j2HRWdIQKWhrfSU8hdATGRtQ2O2w8h0fJD56\n\t/3PJLnFlrIvYfe5Pp5Wk+0ARWDiCdGQxBEkmctf0QXtOMYy8A0qgYI9VGU2yjmwno/im2MUol\n\tAL/7rf/kI9b7bHFW16Kcyu44jaH1NH+kknxebG8IiRX1ps4LOfqoyww7R79hjRmQHQymisQPH\n\teZMCBiAslRtkmmh8/t3sx4B5uP","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: controls: validate all\n\tControlInfo values","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":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]