[libcamera-devel] [PATCH 4/4] android: camera_metadata: Add type sanity check to updateEntry()

[Hirokazu Honda]

Hi Laurent,

On Sun, May 16, 2021 at 3:38 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> The CameraMetadata::updateEntry() functions cast the data pointer to a
> void pointer, which is then used internally to call
> update_camera_metadata_entry(). If the caller passes a pointer to an
> incorrect data type, the behaviour is undefined, with possible crashes
> if the incorrect data type is smaller than expected by the Android
> metadata library.
>
> To avoid crashes, make all public updateEntry() functions take typed
> pointers, and pass the element size to the internal function. The
> element size is then checked against the expected size, and an error
> message logged if they don't match. This won't catch incorrect data
> types that have the same size as the correct type, but will at least
> avoid potential crashes.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/android/camera_metadata.cpp | 11 ++++++++++-
>  src/android/camera_metadata.h   | 12 +++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/android/camera_metadata.cpp
> b/src/android/camera_metadata.cpp
> index 59366c50cc16..3eb71d7f7d9d 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -137,7 +137,8 @@ bool CameraMetadata::addEntry(uint32_t tag, const void
> *data, size_t count,
>         return false;
>  }
>
> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
> count)
> +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
> count,
> +                                size_t elementSize)
>  {
>         if (!valid_)
>                 return false;
> @@ -152,6 +153,14 @@ bool CameraMetadata::updateEntry(uint32_t tag, const
> void *data, size_t count)
>                 return false;
>         }
>
> +       if (camera_metadata_type_size[entry.type] != elementSize) {
> +               const char *name = get_camera_metadata_tag_name(tag);
> +               LOG(CameraMetadata, Error)
> +                       << "Invalid element size for tag "
> +                       << (name ? name : "<unknown>") << ": not present";
>

Should the sizes be printed too?

Regardless of that,
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


> +               return false;
> +       }
> +
>         size_t oldSize =
>                 calculate_camera_metadata_entry_data_size(entry.type,
>                                                           entry.count);
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index d7c8d9df689f..f8f2a0d23aa3 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -55,7 +55,7 @@ public:
>         template<typename T>
>         bool updateEntry(uint32_t tag, const T &data)
>         {
> -               return updateEntry(tag, &data, 1);
> +               return updateEntry(tag, &data, 1, sizeof(T));
>         }
>
>         template<typename T, size_t size>
> @@ -68,10 +68,14 @@ public:
>                  typename T = typename S::value_type>
>         bool updateEntry(uint32_t tag, const S &data)
>         {
> -               return updateEntry(tag, data.data(), data.size());
> +               return updateEntry(tag, data.data(), data.size(),
> sizeof(T));
>         }
>
> -       bool updateEntry(uint32_t tag, const void *data, size_t count);
> +       template<typename T>
> +       bool updateEntry(uint32_t tag, const T *data, size_t count)
> +       {
> +               return updateEntry(tag, data, count, sizeof(T));
> +       }
>
>         camera_metadata_t *get();
>         const camera_metadata_t *get() const;
> @@ -80,6 +84,8 @@ private:
>         bool resize(size_t count, size_t size);
>         bool addEntry(uint32_t tag, const void *data, size_t count,
>                       size_t elementSize);
> +       bool updateEntry(uint32_t tag, const void *data, size_t count,
> +                        size_t elementSize);
>
>         camera_metadata_t *metadata_;
>         bool valid_;
> --
> Regards,
>
> Laurent Pinchart
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210517/ed2dda6e/attachment.htm>