| Message ID | 20210515183826.21003-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sun, May 16, 2021 at 3:38 AM Laurent Pinchart < laurent.pinchart@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@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@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 > >
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@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@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_; >
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@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_;
Hi Hiro, On Mon, May 17, 2021 at 12:43:55PM +0900, Hirokazu Honda wrote: > On Sun, May 16, 2021 at 3:38 AM 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@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? I've considered that, but I don't think it would be that useful. The size from camera_metadata_type_size is easy to look up. > Regardless of that, > Reviewed-by: Hirokazu Honda <hiroh@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_;
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@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@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_;
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"; + 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_;
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@ideasonboard.com> --- src/android/camera_metadata.cpp | 11 ++++++++++- src/android/camera_metadata.h | 12 +++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-)