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

[paul.elder at ideasonboard.com]

Hi Laurent,

On Sat, May 15, 2021 at 09:38:26PM +0300, 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)
> +			<< "Invalid element size for tag "
> +			<< (name ? name : "<unknown>") << ": not present";

I think s/ << ": not present"//, since that must've been copy and pasted
from the other error message about a tag that couldn't be found.


Thanks,

Paul

> +		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_;