[{"id":16968,"web_url":"https://patchwork.libcamera.org/comment/16968/","msgid":"","date":"2021-05-17T03:43:55","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Sun, May 16, 2021 at 3:38 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> The CameraMetadata::updateEntry() functions cast the data pointer to a\n> void pointer, which is then used internally to call\n> update_camera_metadata_entry(). If the caller passes a pointer to an\n> incorrect data type, the behaviour is undefined, with possible crashes\n> if the incorrect data type is smaller than expected by the Android\n> metadata library.\n>\n> To avoid crashes, make all public updateEntry() functions take typed\n> pointers, and pass the element size to the internal function. The\n> element size is then checked against the expected size, and an error\n> message logged if they don't match. This won't catch incorrect data\n> types that have the same size as the correct type, but will at least\n> avoid potential crashes.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_metadata.cpp | 11 ++++++++++-\n> src/android/camera_metadata.h | 12 +++++++++---\n> 2 files changed, 19 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/android/camera_metadata.cpp\n> b/src/android/camera_metadata.cpp\n> index 59366c50cc16..3eb71d7f7d9d 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void\n> *data, size_t count,\n> return false;\n> }\n>\n> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n> count)\n> +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n> count,\n> + size_t elementSize)\n> {\n> if (!valid_)\n> return false;\n> @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n> void *data, size_t count)\n> return false;\n> }\n>\n> + if (camera_metadata_type_size[entry.type] != elementSize) {\n> + const char *name = get_camera_metadata_tag_name(tag);\n> + LOG(CameraMetadata, Error)\n> + << \"Invalid element size for tag \"\n> + << (name ? name : \"\") << \": not present\";\n>\n\nShould the sizes be printed too?\n\nRegardless of that,\nReviewed-by: Hirokazu Honda \n\n\n> + return false;\n> + }\n> +\n> size_t oldSize =\n> calculate_camera_metadata_entry_data_size(entry.type,\n> entry.count);\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index d7c8d9df689f..f8f2a0d23aa3 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -55,7 +55,7 @@ public:\n> template\n> bool updateEntry(uint32_t tag, const T &data)\n> {\n> - return updateEntry(tag, &data, 1);\n> + return updateEntry(tag, &data, 1, sizeof(T));\n> }\n>\n> template\n> @@ -68,10 +68,14 @@ public:\n> typename T = typename S::value_type>\n> bool updateEntry(uint32_t tag, const S &data)\n> {\n> - return updateEntry(tag, data.data(), data.size());\n> + return updateEntry(tag, data.data(), data.size(),\n> sizeof(T));\n> }\n>\n> - bool updateEntry(uint32_t tag, const void *data, size_t count);\n> + template\n> + bool updateEntry(uint32_t tag, const T *data, size_t count)\n> + {\n> + return updateEntry(tag, data, count, sizeof(T));\n> + }\n>\n> camera_metadata_t *get();\n> const camera_metadata_t *get() const;\n> @@ -80,6 +84,8 @@ private:\n> bool resize(size_t count, size_t size);\n> bool addEntry(uint32_t tag, const void *data, size_t count,\n> size_t elementSize);\n> + bool updateEntry(uint32_t tag, const void *data, size_t count,\n> + size_t elementSize);\n>\n> camera_metadata_t *metadata_;\n> bool valid_;\n> --\n> Regards,\n>\n> Laurent Pinchart\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 EE674C31FC\n\tfor ;\n\tMon, 17 May 2021 03:44:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 342CC6891E;\n\tMon, 17 May 2021 05:44:07 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF29E602B1\n\tfor ;\n\tMon, 17 May 2021 05:44:05 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id n2so6981452ejy.7\n\tfor ;\n\tSun, 16 May 2021 20:44:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"IUVeAOa3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=58yRBAF9k96Rcj8jmrJeKpGfkUokgPw0k6c28zE+9wU=;\n\tb=IUVeAOa3mEWsFhi/AxZ1tOu+UMDn7RmJ8GLrwHYjXVFW0pzRyQQ2umGFtsPhgyMwEW\n\tvfvA7ZrWqXCr4docs9DfGCCORjoQheyiv6kiKsfY42IN4w+l/jXy2xik1hf71Dmhutii\n\teLKSu844a1LJFtspUezUtd/HacXmVc9pQhilU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=58yRBAF9k96Rcj8jmrJeKpGfkUokgPw0k6c28zE+9wU=;\n\tb=KoiUe0VARrGASYzq53r9fL6CIRbD9bjCstqZ+BCagsn4zZOtYEXKb7IWCg/ZXyd73t\n\tIPwSFOiVtjLBnG4oUvc2cMHofrf+ixAcfSav6Q19w9ghuM1hZoTsYc+P6KOx/Y/t+NSx\n\t7PeefumPvw+bBEwpfrv9YfzaYOvM0XJ9TP91dM+1EnOCOCDKZpTm9qT6mwH6SRt+BQpN\n\tPUVB07a1J2vaZo97esVxW4Yoy4PDNFgr0VJcvTNq61hOrkJTQSArGVOpZ7EpvEvQed07\n\tcKZdt3GpFi7nACP6SuExzVIQapqxpyhewToK8y7qplaSmOTcZVTxg9Ds2b9ke/PlFr2d\n\toGQw==","X-Gm-Message-State":"AOAM532JZQvtOGhw9yxMuAKWvNu09WxdcHvpyFLICW7snxo5KrNsVPFw\n\tSYKLDo/4SaB8Rzm6p45Y+EjN4mKSFIOSzSfpq5eOWw==","X-Google-Smtp-Source":"ABdhPJw9VHFMXQj2N8dCGxgw6pqMT+b+l/Rj/m9BpL4ghBbApdPPc5gd0slqGILoEklHdHDfTFCze6SeeNXiQdlXUY8=","X-Received":"by 2002:a17:906:ecf9:: with SMTP id\n\tqt25mr6424475ejb.55.1621223045391; \n\tSun, 16 May 2021 20:44:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210515183826.21003-1-laurent.pinchart@ideasonboard.com>\n\t<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 17 May 2021 12:43:55 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000b8fa0905c27e6710\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16988,"web_url":"https://patchwork.libcamera.org/comment/16988/","msgid":"<14a8e407-ca48-7f69-0c87-c87d868914ed@ideasonboard.com>","date":"2021-05-17T09:43:19","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 15/05/2021 19:38, Laurent Pinchart wrote:\n> The CameraMetadata::updateEntry() functions cast the data pointer to a\n> void pointer, which is then used internally to call\n> update_camera_metadata_entry(). If the caller passes a pointer to an\n> incorrect data type, the behaviour is undefined, with possible crashes\n> if the incorrect data type is smaller than expected by the Android\n> metadata library.\n> \n> To avoid crashes, make all public updateEntry() functions take typed\n> pointers, and pass the element size to the internal function. The\n> element size is then checked against the expected size, and an error\n> message logged if they don't match. This won't catch incorrect data\n> types that have the same size as the correct type, but will at least\n> avoid potential crashes.\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_metadata.cpp | 11 ++++++++++-\n> src/android/camera_metadata.h | 12 +++++++++---\n> 2 files changed, 19 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 59366c50cc16..3eb71d7f7d9d 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> \treturn false;\n> }\n> \n> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t\t size_t elementSize)\n> {\n> \tif (!valid_)\n> \t\treturn false;\n> @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> \t\treturn false;\n> \t}\n> \n> +\tif (camera_metadata_type_size[entry.type] != elementSize) {\n> +\t\tconst char *name = get_camera_metadata_tag_name(tag);\n> +\t\tLOG(CameraMetadata, Error)\n\nThis sounds like the sort of thing that would not normally occur except\nduring development or when code is changed.\n\nShould this be Fatal to cause an assert in development builds?\nOr otherwise, perhaps just add an\n\nASSERT(camera_metadata_type_size[entry.type] == elementSize);\noutside of the conditional?\n\n(Perhaps I should re-visit making Fatal non-fatal in release builds...)\n\nBut with or without that:\n\nReviewed-by: Kieran Bingham \n\n\n> +\t\t\t<< \"Invalid element size for tag \"\n> +\t\t\t<< (name ? name : \"\") << \": not present\";\n> +\t\treturn false;\n> +\t}\n> +\n> \tsize_t oldSize =\n> \t\tcalculate_camera_metadata_entry_data_size(entry.type,\n> \t\t\t\t\t\t\t entry.count);\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index d7c8d9df689f..f8f2a0d23aa3 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -55,7 +55,7 @@ public:\n> \ttemplate\n> \tbool updateEntry(uint32_t tag, const T &data)\n> \t{\n> -\t\treturn updateEntry(tag, &data, 1);\n> +\t\treturn updateEntry(tag, &data, 1, sizeof(T));\n> \t}\n> \n> \ttemplate\n> @@ -68,10 +68,14 @@ public:\n> \t\t typename T = typename S::value_type>\n> \tbool updateEntry(uint32_t tag, const S &data)\n> \t{\n> -\t\treturn updateEntry(tag, data.data(), data.size());\n> +\t\treturn updateEntry(tag, data.data(), data.size(), sizeof(T));\n> \t}\n> \n> -\tbool updateEntry(uint32_t tag, const void *data, size_t count);\n> +\ttemplate\n> +\tbool updateEntry(uint32_t tag, const T *data, size_t count)\n> +\t{\n> +\t\treturn updateEntry(tag, data, count, sizeof(T));\n> +\t}\n> \n> \tcamera_metadata_t *get();\n> \tconst camera_metadata_t *get() const;\n> @@ -80,6 +84,8 @@ private:\n> \tbool resize(size_t count, size_t size);\n> \tbool addEntry(uint32_t tag, const void *data, size_t count,\n> \t\t size_t elementSize);\n> +\tbool updateEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t size_t elementSize);\n> \n> \tcamera_metadata_t *metadata_;\n> \tbool valid_;\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 91457C31FB\n\tfor ;\n\tMon, 17 May 2021 09:43:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0632D6891F;\n\tMon, 17 May 2021 11:43:25 +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 9E108602B2\n\tfor ;\n\tMon, 17 May 2021 11:43:23 +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 CDB5788F;\n\tMon, 17 May 2021 11:43:22 +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=\"IXBy5NDp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621244603;\n\tbh=WvPxAgvWiL5/QQ1wQbiuTWWm/8b25kvVI/JVrA86TAU=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=IXBy5NDp0mY+s7UpSGHDy8ZVxL8fa1lNBbDwYgBvcuWrP68cW6NuWy9Q9WJS0Nkv3\n\tziaL5CB4sZcLSCY0M4b6rjDJumUAiry95Gdpnu4MSInojSwlNXKNdl1djiwD3AZVQR\n\tWxODWq/2N21J2oAyxYJKTxkoggyQtsbZwQ8JuN/4=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210515183826.21003-1-laurent.pinchart@ideasonboard.com>\n\t<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<14a8e407-ca48-7f69-0c87-c87d868914ed@ideasonboard.com>","Date":"Mon, 17 May 2021 10:43:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","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","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16997,"web_url":"https://patchwork.libcamera.org/comment/16997/","msgid":"<20210518093413.GA195599@pyrite.rasen.tech>","date":"2021-05-18T09:34:13","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Sat, May 15, 2021 at 09:38:26PM +0300, Laurent Pinchart wrote:\n> The CameraMetadata::updateEntry() functions cast the data pointer to a\n> void pointer, which is then used internally to call\n> update_camera_metadata_entry(). If the caller passes a pointer to an\n> incorrect data type, the behaviour is undefined, with possible crashes\n> if the incorrect data type is smaller than expected by the Android\n> metadata library.\n> \n> To avoid crashes, make all public updateEntry() functions take typed\n> pointers, and pass the element size to the internal function. The\n> element size is then checked against the expected size, and an error\n> message logged if they don't match. This won't catch incorrect data\n> types that have the same size as the correct type, but will at least\n> avoid potential crashes.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_metadata.cpp | 11 ++++++++++-\n> src/android/camera_metadata.h | 12 +++++++++---\n> 2 files changed, 19 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 59366c50cc16..3eb71d7f7d9d 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> \treturn false;\n> }\n> \n> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t\t size_t elementSize)\n> {\n> \tif (!valid_)\n> \t\treturn false;\n> @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> \t\treturn false;\n> \t}\n> \n> +\tif (camera_metadata_type_size[entry.type] != elementSize) {\n> +\t\tconst char *name = get_camera_metadata_tag_name(tag);\n> +\t\tLOG(CameraMetadata, Error)\n> +\t\t\t<< \"Invalid element size for tag \"\n> +\t\t\t<< (name ? name : \"\") << \": not present\";\n\nI think s/ << \": not present\"//, since that must've been copy and pasted\nfrom the other error message about a tag that couldn't be found.\n\n\nThanks,\n\nPaul\n\n> +\t\treturn false;\n> +\t}\n> +\n> \tsize_t oldSize =\n> \t\tcalculate_camera_metadata_entry_data_size(entry.type,\n> \t\t\t\t\t\t\t entry.count);\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index d7c8d9df689f..f8f2a0d23aa3 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -55,7 +55,7 @@ public:\n> \ttemplate\n> \tbool updateEntry(uint32_t tag, const T &data)\n> \t{\n> -\t\treturn updateEntry(tag, &data, 1);\n> +\t\treturn updateEntry(tag, &data, 1, sizeof(T));\n> \t}\n> \n> \ttemplate\n> @@ -68,10 +68,14 @@ public:\n> \t\t typename T = typename S::value_type>\n> \tbool updateEntry(uint32_t tag, const S &data)\n> \t{\n> -\t\treturn updateEntry(tag, data.data(), data.size());\n> +\t\treturn updateEntry(tag, data.data(), data.size(), sizeof(T));\n> \t}\n> \n> -\tbool updateEntry(uint32_t tag, const void *data, size_t count);\n> +\ttemplate\n> +\tbool updateEntry(uint32_t tag, const T *data, size_t count)\n> +\t{\n> +\t\treturn updateEntry(tag, data, count, sizeof(T));\n> +\t}\n> \n> \tcamera_metadata_t *get();\n> \tconst camera_metadata_t *get() const;\n> @@ -80,6 +84,8 @@ private:\n> \tbool resize(size_t count, size_t size);\n> \tbool addEntry(uint32_t tag, const void *data, size_t count,\n> \t\t size_t elementSize);\n> +\tbool updateEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t size_t elementSize);\n> \n> \tcamera_metadata_t *metadata_;\n> \tbool valid_;","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 7E5F2C31FB\n\tfor ;\n\tTue, 18 May 2021 09:34:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B5CB68919;\n\tTue, 18 May 2021 11:34:22 +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 D4DE5602B1\n\tfor ;\n\tTue, 18 May 2021 11:34:20 +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 362FB102;\n\tTue, 18 May 2021 11:34:18 +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=\"LRxKS55L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621330460;\n\tbh=/0s+1IpFRaWyy7QHphEvzFmPxq6qktd9L5ITHI+NcgQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LRxKS55LWC5ZLxQL2aSJ7hiVeVzbEg99FruZdJM92GbkFe/4dNW/mgL2DctN7htEU\n\tX3y7tC0MZJXJsGXJssB/RhHVpz+4RBZGLFrHowndT/XZvYKAeY7SQynBzPhCzG9uTy\n\tBMx5YjwP7iptT29Dluzpdevjqo21NUt+qDbUY3zM=","Date":"Tue, 18 May 2021 18:34:13 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210518093413.GA195599@pyrite.rasen.tech>","References":"<20210515183826.21003-1-laurent.pinchart@ideasonboard.com>\n\t<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17004,"web_url":"https://patchwork.libcamera.org/comment/17004/","msgid":"","date":"2021-05-18T10:55:54","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, May 17, 2021 at 12:43:55PM +0900, Hirokazu Honda wrote:\n> On Sun, May 16, 2021 at 3:38 AM Laurent Pinchart wrote:\n> > The CameraMetadata::updateEntry() functions cast the data pointer to a\n> > void pointer, which is then used internally to call\n> > update_camera_metadata_entry(). If the caller passes a pointer to an\n> > incorrect data type, the behaviour is undefined, with possible crashes\n> > if the incorrect data type is smaller than expected by the Android\n> > metadata library.\n> >\n> > To avoid crashes, make all public updateEntry() functions take typed\n> > pointers, and pass the element size to the internal function. The\n> > element size is then checked against the expected size, and an error\n> > message logged if they don't match. This won't catch incorrect data\n> > types that have the same size as the correct type, but will at least\n> > avoid potential crashes.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/android/camera_metadata.cpp | 11 ++++++++++-\n> > src/android/camera_metadata.h | 12 +++++++++---\n> > 2 files changed, 19 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/android/camera_metadata.cpp\n> > b/src/android/camera_metadata.cpp\n> > index 59366c50cc16..3eb71d7f7d9d 100644\n> > --- a/src/android/camera_metadata.cpp\n> > +++ b/src/android/camera_metadata.cpp\n> > @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > return false;\n> > }\n> >\n> > -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> > +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,\n> > + size_t elementSize)\n> > {\n> > if (!valid_)\n> > return false;\n> > @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n> > void *data, size_t count)\n> > return false;\n> > }\n> >\n> > + if (camera_metadata_type_size[entry.type] != elementSize) {\n> > + const char *name = get_camera_metadata_tag_name(tag);\n> > + LOG(CameraMetadata, Error)\n> > + << \"Invalid element size for tag \"\n> > + << (name ? name : \"\") << \": not present\";\n> \n> Should the sizes be printed too?\n\nI've considered that, but I don't think it would be that useful. The\nsize from camera_metadata_type_size is easy to look up.\n\n> Regardless of that,\n> Reviewed-by: Hirokazu Honda \n> \n> > + return false;\n> > + }\n> > +\n> > size_t oldSize =\n> > calculate_camera_metadata_entry_data_size(entry.type,\n> > entry.count);\n> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > index d7c8d9df689f..f8f2a0d23aa3 100644\n> > --- a/src/android/camera_metadata.h\n> > +++ b/src/android/camera_metadata.h\n> > @@ -55,7 +55,7 @@ public:\n> > template\n> > bool updateEntry(uint32_t tag, const T &data)\n> > {\n> > - return updateEntry(tag, &data, 1);\n> > + return updateEntry(tag, &data, 1, sizeof(T));\n> > }\n> >\n> > template\n> > @@ -68,10 +68,14 @@ public:\n> > typename T = typename S::value_type>\n> > bool updateEntry(uint32_t tag, const S &data)\n> > {\n> > - return updateEntry(tag, data.data(), data.size());\n> > + return updateEntry(tag, data.data(), data.size(), sizeof(T));\n> > }\n> >\n> > - bool updateEntry(uint32_t tag, const void *data, size_t count);\n> > + template\n> > + bool updateEntry(uint32_t tag, const T *data, size_t count)\n> > + {\n> > + return updateEntry(tag, data, count, sizeof(T));\n> > + }\n> >\n> > camera_metadata_t *get();\n> > const camera_metadata_t *get() const;\n> > @@ -80,6 +84,8 @@ private:\n> > bool resize(size_t count, size_t size);\n> > bool addEntry(uint32_t tag, const void *data, size_t count,\n> > size_t elementSize);\n> > + bool updateEntry(uint32_t tag, const void *data, size_t count,\n> > + size_t elementSize);\n> >\n> > camera_metadata_t *metadata_;\n> > bool valid_;","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 293F0C31FF\n\tfor ;\n\tTue, 18 May 2021 10:55:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A7C56891E;\n\tTue, 18 May 2021 12:55:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1194F6891C\n\tfor ;\n\tTue, 18 May 2021 12:55:57 +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 72724102;\n\tTue, 18 May 2021 12:55:56 +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=\"NuskrE12\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621335356;\n\tbh=AbwevZ5YjIE8fhpbGeU8Szes1aRwqNGaQWnDFlXWm24=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NuskrE12WpR8BE3G4D2yzbEk0DS9nz0aeBNBuhFYYrVI2Vk8p75xbpzpWc+R0yjS5\n\tXmS/K6C7YHR3Co0NNA+cN3t++TDmRQY4T9gj4iX6MZk7yXWBngHn1PbTKCdtqWrGpE\n\t3JtrMS4SSJ4cCgp3ku/wpcDWURbISjtHOidrWKes=","Date":"Tue, 18 May 2021 13:55:54 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210515183826.21003-1-laurent.pinchart@ideasonboard.com>\n\t<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17005,"web_url":"https://patchwork.libcamera.org/comment/17005/","msgid":"","date":"2021-05-18T10:58:13","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, May 17, 2021 at 10:43:19AM +0100, Kieran Bingham wrote:\n> On 15/05/2021 19:38, Laurent Pinchart wrote:\n> > The CameraMetadata::updateEntry() functions cast the data pointer to a\n> > void pointer, which is then used internally to call\n> > update_camera_metadata_entry(). If the caller passes a pointer to an\n> > incorrect data type, the behaviour is undefined, with possible crashes\n> > if the incorrect data type is smaller than expected by the Android\n> > metadata library.\n> > \n> > To avoid crashes, make all public updateEntry() functions take typed\n> > pointers, and pass the element size to the internal function. The\n> > element size is then checked against the expected size, and an error\n> > message logged if they don't match. This won't catch incorrect data\n> > types that have the same size as the correct type, but will at least\n> > avoid potential crashes.\n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/android/camera_metadata.cpp | 11 ++++++++++-\n> > src/android/camera_metadata.h | 12 +++++++++---\n> > 2 files changed, 19 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > index 59366c50cc16..3eb71d7f7d9d 100644\n> > --- a/src/android/camera_metadata.cpp\n> > +++ b/src/android/camera_metadata.cpp\n> > @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > \treturn false;\n> > }\n> > \n> > -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> > +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,\n> > +\t\t\t\t size_t elementSize)\n> > {\n> > \tif (!valid_)\n> > \t\treturn false;\n> > @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> > \t\treturn false;\n> > \t}\n> > \n> > +\tif (camera_metadata_type_size[entry.type] != elementSize) {\n> > +\t\tconst char *name = get_camera_metadata_tag_name(tag);\n> > +\t\tLOG(CameraMetadata, Error)\n> \n> This sounds like the sort of thing that would not normally occur except\n> during development or when code is changed.\n> \n> Should this be Fatal to cause an assert in development builds?\n> Or otherwise, perhaps just add an\n\nI think it's a good idea, I'll change that.\n\n> ASSERT(camera_metadata_type_size[entry.type] == elementSize);\n> outside of the conditional?\n> \n> (Perhaps I should re-visit making Fatal non-fatal in release builds...)\n> \n> But with or without that:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\t\t\t<< \"Invalid element size for tag \"\n> > +\t\t\t<< (name ? name : \"\") << \": not present\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > \tsize_t oldSize =\n> > \t\tcalculate_camera_metadata_entry_data_size(entry.type,\n> > \t\t\t\t\t\t\t entry.count);\n> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > index d7c8d9df689f..f8f2a0d23aa3 100644\n> > --- a/src/android/camera_metadata.h\n> > +++ b/src/android/camera_metadata.h\n> > @@ -55,7 +55,7 @@ public:\n> > \ttemplate\n> > \tbool updateEntry(uint32_t tag, const T &data)\n> > \t{\n> > -\t\treturn updateEntry(tag, &data, 1);\n> > +\t\treturn updateEntry(tag, &data, 1, sizeof(T));\n> > \t}\n> > \n> > \ttemplate\n> > @@ -68,10 +68,14 @@ public:\n> > \t\t typename T = typename S::value_type>\n> > \tbool updateEntry(uint32_t tag, const S &data)\n> > \t{\n> > -\t\treturn updateEntry(tag, data.data(), data.size());\n> > +\t\treturn updateEntry(tag, data.data(), data.size(), sizeof(T));\n> > \t}\n> > \n> > -\tbool updateEntry(uint32_t tag, const void *data, size_t count);\n> > +\ttemplate\n> > +\tbool updateEntry(uint32_t tag, const T *data, size_t count)\n> > +\t{\n> > +\t\treturn updateEntry(tag, data, count, sizeof(T));\n> > +\t}\n> > \n> > \tcamera_metadata_t *get();\n> > \tconst camera_metadata_t *get() const;\n> > @@ -80,6 +84,8 @@ private:\n> > \tbool resize(size_t count, size_t size);\n> > \tbool addEntry(uint32_t tag, const void *data, size_t count,\n> > \t\t size_t elementSize);\n> > +\tbool updateEntry(uint32_t tag, const void *data, size_t count,\n> > +\t\t\t size_t elementSize);\n> > \n> > \tcamera_metadata_t *metadata_;\n> > \tbool valid_;","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 A7F74C31FB\n\tfor ;\n\tTue, 18 May 2021 10:58:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26AD16891E;\n\tTue, 18 May 2021 12:58:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 175B96891C\n\tfor ;\n\tTue, 18 May 2021 12:58:16 +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 7928E102;\n\tTue, 18 May 2021 12:58:15 +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=\"DDQ+Uk2O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621335495;\n\tbh=CyEtDV23Jal7D9Obs3JYZyohD+0iIeeBIdvJha9/3rY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DDQ+Uk2OiTf90EhFhNQai4NG7yYqiYRQ3WsyEFqKE81EnJnXZNcqXf5fNQCgIk5Tv\n\tDpTWOJFNvlEQZ4BxV7gqsmphg1Aph1m4E/zglJnSfUZQjPYuPM1+o6tK2YSh3cw0gX\n\tHlTwADQGhXnQkKeA1AmR4X3b6ZhaWaGJd8tr/Hhk=","Date":"Tue, 18 May 2021 13:58:13 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210515183826.21003-1-laurent.pinchart@ideasonboard.com>\n\t<20210515183826.21003-5-laurent.pinchart@ideasonboard.com>\n\t<14a8e407-ca48-7f69-0c87-c87d868914ed@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<14a8e407-ca48-7f69-0c87-c87d868914ed@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_metadata: Add\n\ttype sanity check to updateEntry()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]