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

[Laurent Pinchart]

Hi Kieran,

On Mon, May 17, 2021 at 10:43:19AM +0100, Kieran Bingham wrote:
> 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

I think it's a good idea, I'll change that.

> 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,

Laurent Pinchart