[libcamera-devel] android: camera_device: Fix null pointer dereference
diff mbox series

Message ID 20210628064450.3286-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 1684c3f930b2a27884037bc38856477b80cddd50
Headers show
Series
  • [libcamera-devel] android: camera_device: Fix null pointer dereference
Related show

Commit Message

Laurent Pinchart June 28, 2021, 6:44 a.m. UTC 
Commit 7532caa2c77b ("android: camera_device: Reset config_ if
Camera::configure() fails") reworked the configuration sequence to
ensure that the CameraConfiguration pointers gets reset when
configuration fails. This inadvertently causes a null pointer
dereference, as the CameraStream constructor accesses the camera
configuration through CameraDevice::cameraConfiguration() before the
internal config_ pointer is set.

Fix this by passing the configuration pointer explicitly to the
CameraStream constructor.

Fixes: 7532caa2c77b ("android: camera_device: Reset config_ if Camera::configure() fails")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 4 ++--
 src/android/camera_device.h   | 4 ----
 src/android/camera_stream.cpp | 6 +++---
 src/android/camera_stream.h   | 3 ++-
 4 files changed, 7 insertions(+), 10 deletions(-)

Comments

Paul Elder June 28, 2021, 6:52 a.m. UTC | #1 
Hi Laurent,

On Mon, Jun 28, 2021 at 09:44:50AM +0300, Laurent Pinchart wrote:
> Commit 7532caa2c77b ("android: camera_device: Reset config_ if
> Camera::configure() fails") reworked the configuration sequence to
> ensure that the CameraConfiguration pointers gets reset when
> configuration fails. This inadvertently causes a null pointer
> dereference, as the CameraStream constructor accesses the camera
> configuration through CameraDevice::cameraConfiguration() before the
> internal config_ pointer is set.
> 
> Fix this by passing the configuration pointer explicitly to the
> CameraStream constructor.
> 
> Fixes: 7532caa2c77b ("android: camera_device: Reset config_ if Camera::configure() fails")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Tested-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 4 ++--
>  src/android/camera_device.h   | 4 ----
>  src/android/camera_stream.cpp | 6 +++---
>  src/android/camera_stream.h   | 3 ++-
>  4 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 13ee5fab4412..678cde231c63 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -682,8 +682,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		config->addConfiguration(streamConfig.config);
>  
>  		for (auto &stream : streamConfig.streams) {
> -			streams_.emplace_back(this, stream.type, stream.stream,
> -					      config->size() - 1);
> +			streams_.emplace_back(this, config.get(), stream.type,
> +					      stream.stream, config->size() - 1);
>  			stream.stream->priv = static_cast<void *>(&streams_.back());
>  		}
>  	}
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 18cf51189e90..3361918d4484 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,10 +48,6 @@ public:
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
>  	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> -	libcamera::CameraConfiguration *cameraConfiguration() const
> -	{
> -		return config_.get();
> -	}
>  
>  	const std::string &maker() const { return maker_; }
>  	const std::string &model() const { return model_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index b2f03b505199..bf4a7b41a70a 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -39,10 +39,10 @@ LOG_DECLARE_CATEGORY(HAL)
>   * and buffer allocation.
>   */
>  
> -CameraStream::CameraStream(CameraDevice *const cameraDevice, Type type,
> +CameraStream::CameraStream(CameraDevice *const cameraDevice,
> +			   CameraConfiguration *config, Type type,
>  			   camera3_stream_t *camera3Stream, unsigned int index)
> -	: cameraDevice_(cameraDevice),
> -	  config_(cameraDevice->cameraConfiguration()), type_(type),
> +	: cameraDevice_(cameraDevice), config_(config), type_(type),
>  	  camera3Stream_(camera3Stream), index_(index)
>  {
>  	if (type_ == Type::Internal || type_ == Type::Mapped) {
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 3401672233ca..8ecc6e345414 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -110,7 +110,8 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(CameraDevice *const cameraDevice, Type type,
> +	CameraStream(CameraDevice *const cameraDevice,
> +		     libcamera::CameraConfiguration *config, Type type,
>  		     camera3_stream_t *camera3Stream, unsigned int index);
>  
>  	Type type() const { return type_; }
> -- 
> Regards,
> 
> Laurent Pinchart
>
Umang Jain June 28, 2021, 6:53 a.m. UTC | #2 
Hi Laurent,

Thanks for the fix!

On 6/28/21 12:14 PM, Laurent Pinchart wrote:
> Commit 7532caa2c77b ("android: camera_device: Reset config_ if
> Camera::configure() fails") reworked the configuration sequence to
> ensure that the CameraConfiguration pointers gets reset when
> configuration fails. This inadvertently causes a null pointer
> dereference, as the CameraStream constructor accesses the camera
> configuration through CameraDevice::cameraConfiguration() before the
> internal config_ pointer is set.
>
> Fix this by passing the configuration pointer explicitly to the
> CameraStream constructor.
>
> Fixes: 7532caa2c77b ("android: camera_device: Reset config_ if Camera::configure() fails")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Tested-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/android/camera_device.cpp | 4 ++--
>   src/android/camera_device.h   | 4 ----
>   src/android/camera_stream.cpp | 6 +++---
>   src/android/camera_stream.h   | 3 ++-
>   4 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 13ee5fab4412..678cde231c63 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -682,8 +682,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   		config->addConfiguration(streamConfig.config);
>   
>   		for (auto &stream : streamConfig.streams) {
> -			streams_.emplace_back(this, stream.type, stream.stream,
> -					      config->size() - 1);
> +			streams_.emplace_back(this, config.get(), stream.type,
> +					      stream.stream, config->size() - 1);
>   			stream.stream->priv = static_cast<void *>(&streams_.back());
>   		}
>   	}
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 18cf51189e90..3361918d4484 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,10 +48,6 @@ public:
>   	unsigned int id() const { return id_; }
>   	camera3_device_t *camera3Device() { return &camera3Device_; }
>   	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> -	libcamera::CameraConfiguration *cameraConfiguration() const
> -	{
> -		return config_.get();
> -	}
>   
>   	const std::string &maker() const { return maker_; }
>   	const std::string &model() const { return model_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index b2f03b505199..bf4a7b41a70a 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -39,10 +39,10 @@ LOG_DECLARE_CATEGORY(HAL)
>    * and buffer allocation.
>    */
>   
> -CameraStream::CameraStream(CameraDevice *const cameraDevice, Type type,
> +CameraStream::CameraStream(CameraDevice *const cameraDevice,
> +			   CameraConfiguration *config, Type type,
>   			   camera3_stream_t *camera3Stream, unsigned int index)
> -	: cameraDevice_(cameraDevice),
> -	  config_(cameraDevice->cameraConfiguration()), type_(type),
> +	: cameraDevice_(cameraDevice), config_(config), type_(type),
>   	  camera3Stream_(camera3Stream), index_(index)
>   {
>   	if (type_ == Type::Internal || type_ == Type::Mapped) {
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 3401672233ca..8ecc6e345414 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -110,7 +110,8 @@ public:
>   		Internal,
>   		Mapped,
>   	};
> -	CameraStream(CameraDevice *const cameraDevice, Type type,
> +	CameraStream(CameraDevice *const cameraDevice,
> +		     libcamera::CameraConfiguration *config, Type type,
>   		     camera3_stream_t *camera3Stream, unsigned int index);
>   
>   	Type type() const { return type_; }
Hirokazu Honda June 28, 2021, 7:31 a.m. UTC | #3 
Hi Laurent,

On Mon, Jun 28, 2021 at 3:52 PM <paul.elder@ideasonboard.com> wrote:
>
> Hi Laurent,
>
> On Mon, Jun 28, 2021 at 09:44:50AM +0300, Laurent Pinchart wrote:
> > Commit 7532caa2c77b ("android: camera_device: Reset config_ if
> > Camera::configure() fails") reworked the configuration sequence to
> > ensure that the CameraConfiguration pointers gets reset when
> > configuration fails. This inadvertently causes a null pointer
> > dereference, as the CameraStream constructor accesses the camera
> > configuration through CameraDevice::cameraConfiguration() before the
> > internal config_ pointer is set.
> >
> > Fix this by passing the configuration pointer explicitly to the
> > CameraStream constructor.
> >
> > Fixes: 7532caa2c77b ("android: camera_device: Reset config_ if Camera::configure() fails")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Tested-by: Paul Elder <paul.elder@ideasonboard.com>

Oops, I couldn't catch this in the previous review. :(
Thanks for fixing.

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>
> > ---
> >  src/android/camera_device.cpp | 4 ++--
> >  src/android/camera_device.h   | 4 ----
> >  src/android/camera_stream.cpp | 6 +++---
> >  src/android/camera_stream.h   | 3 ++-
> >  4 files changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 13ee5fab4412..678cde231c63 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -682,8 +682,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >               config->addConfiguration(streamConfig.config);
> >
> >               for (auto &stream : streamConfig.streams) {
> > -                     streams_.emplace_back(this, stream.type, stream.stream,
> > -                                           config->size() - 1);
> > +                     streams_.emplace_back(this, config.get(), stream.type,
> > +                                           stream.stream, config->size() - 1);
> >                       stream.stream->priv = static_cast<void *>(&streams_.back());
> >               }
> >       }
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 18cf51189e90..3361918d4484 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -48,10 +48,6 @@ public:
> >       unsigned int id() const { return id_; }
> >       camera3_device_t *camera3Device() { return &camera3Device_; }
> >       const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> > -     libcamera::CameraConfiguration *cameraConfiguration() const
> > -     {
> > -             return config_.get();
> > -     }
> >
> >       const std::string &maker() const { return maker_; }
> >       const std::string &model() const { return model_; }
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index b2f03b505199..bf4a7b41a70a 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -39,10 +39,10 @@ LOG_DECLARE_CATEGORY(HAL)
> >   * and buffer allocation.
> >   */
> >
> > -CameraStream::CameraStream(CameraDevice *const cameraDevice, Type type,
> > +CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > +                        CameraConfiguration *config, Type type,
> >                          camera3_stream_t *camera3Stream, unsigned int index)
> > -     : cameraDevice_(cameraDevice),
> > -       config_(cameraDevice->cameraConfiguration()), type_(type),
> > +     : cameraDevice_(cameraDevice), config_(config), type_(type),
> >         camera3Stream_(camera3Stream), index_(index)
> >  {
> >       if (type_ == Type::Internal || type_ == Type::Mapped) {
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 3401672233ca..8ecc6e345414 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -110,7 +110,8 @@ public:
> >               Internal,
> >               Mapped,
> >       };
> > -     CameraStream(CameraDevice *const cameraDevice, Type type,
> > +     CameraStream(CameraDevice *const cameraDevice,
> > +                  libcamera::CameraConfiguration *config, Type type,
> >                    camera3_stream_t *camera3Stream, unsigned int index);
> >
> >       Type type() const { return type_; }
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 13ee5fab4412..678cde231c63 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -682,8 +682,8 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		config->addConfiguration(streamConfig.config);
 
 		for (auto &stream : streamConfig.streams) {
-			streams_.emplace_back(this, stream.type, stream.stream,
-					      config->size() - 1);
+			streams_.emplace_back(this, config.get(), stream.type,
+					      stream.stream, config->size() - 1);
 			stream.stream->priv = static_cast<void *>(&streams_.back());
 		}
 	}
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 18cf51189e90..3361918d4484 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -48,10 +48,6 @@  public:
 	unsigned int id() const { return id_; }
 	camera3_device_t *camera3Device() { return &camera3Device_; }
 	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
-	libcamera::CameraConfiguration *cameraConfiguration() const
-	{
-		return config_.get();
-	}
 
 	const std::string &maker() const { return maker_; }
 	const std::string &model() const { return model_; }
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index b2f03b505199..bf4a7b41a70a 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -39,10 +39,10 @@  LOG_DECLARE_CATEGORY(HAL)
  * and buffer allocation.
  */
 
-CameraStream::CameraStream(CameraDevice *const cameraDevice, Type type,
+CameraStream::CameraStream(CameraDevice *const cameraDevice,
+			   CameraConfiguration *config, Type type,
 			   camera3_stream_t *camera3Stream, unsigned int index)
-	: cameraDevice_(cameraDevice),
-	  config_(cameraDevice->cameraConfiguration()), type_(type),
+	: cameraDevice_(cameraDevice), config_(config), type_(type),
 	  camera3Stream_(camera3Stream), index_(index)
 {
 	if (type_ == Type::Internal || type_ == Type::Mapped) {
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 3401672233ca..8ecc6e345414 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -110,7 +110,8 @@  public:
 		Internal,
 		Mapped,
 	};
-	CameraStream(CameraDevice *const cameraDevice, Type type,
+	CameraStream(CameraDevice *const cameraDevice,
+		     libcamera::CameraConfiguration *config, Type type,
 		     camera3_stream_t *camera3Stream, unsigned int index);
 
 	Type type() const { return type_; }