| Message ID | 20190716054218.22136-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 124336329c11e1fc1687504fc37f67189a44ee2d |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2019-07-16 08:42:18 +0300, Laurent Pinchart wrote: > The internal buffers between the CIO2 and ImgU are freed by the > CIO2Device::stop() method, which is called first when stopping > streaming. The ImgUDevice::stop() method is then called, and attempts to > report completion for all queued buffers, which we have just freed. The > use-after-free corrupts memory, leading to crashes. > > Fix this by moving the vector of internal buffers to the IPU3CameraData > where it belongs, and free the buffers after stopping both devices. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index febc867b4d7e..159a9312f95e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -122,7 +122,7 @@ public: > BufferPool *exportBuffers(); > void freeBuffers(); > > - int start(); > + int start(std::vector<std::unique_ptr<Buffer>> *buffer); > int stop(); > > static int mediaBusToFormat(unsigned int code); > @@ -132,7 +132,6 @@ public: > CameraSensor *sensor_; > > BufferPool pool_; > - std::vector<std::unique_ptr<Buffer>> buffers_; > }; > > class IPU3Stream : public Stream > @@ -165,6 +164,8 @@ public: > > IPU3Stream outStream_; > IPU3Stream vfStream_; > + > + std::vector<std::unique_ptr<Buffer>> rawBuffers_; > }; > > class IPU3CameraConfiguration : public CameraConfiguration > @@ -688,7 +689,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > */ > - ret = cio2->start(); > + ret = cio2->start(&data->rawBuffers_); > if (ret) > goto error; > > @@ -704,6 +705,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > error: > LOG(IPU3, Error) << "Failed to start camera " << camera->name(); > > + data->rawBuffers_.clear(); > return ret; > } > > @@ -717,6 +719,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " > << camera->name(); > + > + data->rawBuffers_.clear(); > } > > int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) > @@ -1454,26 +1458,18 @@ void CIO2Device::freeBuffers() > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > } > > -int CIO2Device::start() > +int CIO2Device::start(std::vector<std::unique_ptr<Buffer>> *buffers) > { > - int ret; > - > - buffers_ = output_->queueAllBuffers(); > - if (buffers_.empty()) > + *buffers = output_->queueAllBuffers(); > + if (buffers->empty()) > return -EINVAL; > > - ret = output_->streamOn(); > - if (ret) > - return ret; > - > - return 0; > + return output_->streamOn(); > } > > int CIO2Device::stop() > { > - int ret = output_->streamOff(); > - buffers_.clear(); > - return ret; > + return output_->streamOff(); > } > > int CIO2Device::mediaBusToFormat(unsigned int code) > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for the patch. On Tue, Jul 16, 2019 at 08:42:18AM +0300, Laurent Pinchart wrote: > The internal buffers between the CIO2 and ImgU are freed by the > CIO2Device::stop() method, which is called first when stopping > streaming. The ImgUDevice::stop() method is then called, and attempts to > report completion for all queued buffers, which we have just freed. The > use-after-free corrupts memory, leading to crashes. > > Fix this by moving the vector of internal buffers to the IPU3CameraData > where it belongs, and free the buffers after stopping both devices. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index febc867b4d7e..159a9312f95e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -122,7 +122,7 @@ public: > BufferPool *exportBuffers(); > void freeBuffers(); > > - int start(); > + int start(std::vector<std::unique_ptr<Buffer>> *buffer); s/buffer/buffers To match the definition that you have below. Other than than, looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > int stop(); > > static int mediaBusToFormat(unsigned int code); > @@ -132,7 +132,6 @@ public: > CameraSensor *sensor_; > > BufferPool pool_; > - std::vector<std::unique_ptr<Buffer>> buffers_; > }; > > class IPU3Stream : public Stream > @@ -165,6 +164,8 @@ public: > > IPU3Stream outStream_; > IPU3Stream vfStream_; > + > + std::vector<std::unique_ptr<Buffer>> rawBuffers_; > }; > > class IPU3CameraConfiguration : public CameraConfiguration > @@ -688,7 +689,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > */ > - ret = cio2->start(); > + ret = cio2->start(&data->rawBuffers_); > if (ret) > goto error; > > @@ -704,6 +705,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > error: > LOG(IPU3, Error) << "Failed to start camera " << camera->name(); > > + data->rawBuffers_.clear(); > return ret; > } > > @@ -717,6 +719,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " > << camera->name(); > + > + data->rawBuffers_.clear(); > } > > int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) > @@ -1454,26 +1458,18 @@ void CIO2Device::freeBuffers() > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > } > > -int CIO2Device::start() > +int CIO2Device::start(std::vector<std::unique_ptr<Buffer>> *buffers) > { > - int ret; > - > - buffers_ = output_->queueAllBuffers(); > - if (buffers_.empty()) > + *buffers = output_->queueAllBuffers(); > + if (buffers->empty()) > return -EINVAL; > > - ret = output_->streamOn(); > - if (ret) > - return ret; > - > - return 0; > + return output_->streamOn(); > } > > int CIO2Device::stop() > { > - int ret = output_->streamOff(); > - buffers_.clear(); > - return ret; > + return output_->streamOff(); > } > > int CIO2Device::mediaBusToFormat(unsigned int code) > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index febc867b4d7e..159a9312f95e 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -122,7 +122,7 @@ public: BufferPool *exportBuffers(); void freeBuffers(); - int start(); + int start(std::vector<std::unique_ptr<Buffer>> *buffer); int stop(); static int mediaBusToFormat(unsigned int code); @@ -132,7 +132,6 @@ public: CameraSensor *sensor_; BufferPool pool_; - std::vector<std::unique_ptr<Buffer>> buffers_; }; class IPU3Stream : public Stream @@ -165,6 +164,8 @@ public: IPU3Stream outStream_; IPU3Stream vfStream_; + + std::vector<std::unique_ptr<Buffer>> rawBuffers_; }; class IPU3CameraConfiguration : public CameraConfiguration @@ -688,7 +689,7 @@ int PipelineHandlerIPU3::start(Camera *camera) * Start the ImgU video devices, buffers will be queued to the * ImgU output and viewfinder when requests will be queued. */ - ret = cio2->start(); + ret = cio2->start(&data->rawBuffers_); if (ret) goto error; @@ -704,6 +705,7 @@ int PipelineHandlerIPU3::start(Camera *camera) error: LOG(IPU3, Error) << "Failed to start camera " << camera->name(); + data->rawBuffers_.clear(); return ret; } @@ -717,6 +719,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->name(); + + data->rawBuffers_.clear(); } int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) @@ -1454,26 +1458,18 @@ void CIO2Device::freeBuffers() LOG(IPU3, Error) << "Failed to release CIO2 buffers"; } -int CIO2Device::start() +int CIO2Device::start(std::vector<std::unique_ptr<Buffer>> *buffers) { - int ret; - - buffers_ = output_->queueAllBuffers(); - if (buffers_.empty()) + *buffers = output_->queueAllBuffers(); + if (buffers->empty()) return -EINVAL; - ret = output_->streamOn(); - if (ret) - return ret; - - return 0; + return output_->streamOn(); } int CIO2Device::stop() { - int ret = output_->streamOff(); - buffers_.clear(); - return ret; + return output_->streamOff(); } int CIO2Device::mediaBusToFormat(unsigned int code)
The internal buffers between the CIO2 and ImgU are freed by the CIO2Device::stop() method, which is called first when stopping streaming. The ImgUDevice::stop() method is then called, and attempts to report completion for all queued buffers, which we have just freed. The use-after-free corrupts memory, leading to crashes. Fix this by moving the vector of internal buffers to the IPU3CameraData where it belongs, and free the buffers after stopping both devices. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)