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

[Kieran Bingham]

On 15/05/2021 19:38, Laurent Pinchart 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)

This sounds like the sort of thing that would not normally occur except
during development or when code is changed.

Should this be Fatal to cause an assert in development builds?
Or otherwise, perhaps just add an

ASSERT(camera_metadata_type_size[entry.type] == elementSize);
outside of the conditional?

(Perhaps I should re-visit making Fatal non-fatal in release builds...)

But with or without that:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +			<< "Invalid element size for tag "
> +			<< (name ? name : "<unknown>") << ": not present";
> +		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
--
Kieran