| Message ID | 20230130180244.2150205-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran On Mon, Jan 30, 2023 at 06:02:43PM +0000, Kieran Bingham via libcamera-devel wrote: > Invalid, or not correctly reset requests can cause undefined behaviour > in the pipeline handlers due to unexpected request state. > > If the status has not been reset to Request::RequestPending, it is > either not new, or has not been correctly procesed through > Request::reuse(). > > This can be caught early by validating the status of the request when it > is queued to a camera. > > Reject invalid requests before processing them in the pipeline handlers. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 0da167a7bc51..48bf19b28979 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1126,6 +1126,11 @@ int Camera::queueRequest(Request *request) > return -EXDEV; > } > > + if (request->status() != Request::RequestPending) { > + LOG(Camera, Error) << request->toString() << " is not prepared"; > + return -EINVAL; > + } > + Looks useful Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle > -- > 2.34.1 >
On Mon, Jan 30, 2023 at 06:02:43PM +0000, Kieran Bingham via libcamera-devel wrote: > Invalid, or not correctly reset requests can cause undefined behaviour > in the pipeline handlers due to unexpected request state. > > If the status has not been reset to Request::RequestPending, it is > either not new, or has not been correctly procesed through > Request::reuse(). > > This can be caught early by validating the status of the request when it > is queued to a camera. > > Reject invalid requests before processing them in the pipeline handlers. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/camera.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 0da167a7bc51..48bf19b28979 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1126,6 +1126,11 @@ int Camera::queueRequest(Request *request) > return -EXDEV; > } > > + if (request->status() != Request::RequestPending) { > + LOG(Camera, Error) << request->toString() << " is not prepared"; > + return -EINVAL; > + } > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle > -- > 2.34.1 >
Hi Kieran, Thank you for the patch. On Mon, Jan 30, 2023 at 06:02:43PM +0000, Kieran Bingham via libcamera-devel wrote: > Invalid, or not correctly reset requests can cause undefined behaviour > in the pipeline handlers due to unexpected request state. > > If the status has not been reset to Request::RequestPending, it is > either not new, or has not been correctly procesed through s/procesed/processed/ or maybe s/procesed/reset/ > Request::reuse(). > > This can be caught early by validating the status of the request when it > is queued to a camera. > > Reject invalid requests before processing them in the pipeline handlers. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 0da167a7bc51..48bf19b28979 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1126,6 +1126,11 @@ int Camera::queueRequest(Request *request) > return -EXDEV; > } > > + if (request->status() != Request::RequestPending) { > + LOG(Camera, Error) << request->toString() << " is not prepared"; "prepared" is the wrong word here. We have a prepared concept for requests (see Request::Private::prepare()), and it's not related to this. The same comment applies to the commit message. Other than this, the patch looks good to me. > + return -EINVAL; > + } > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 0da167a7bc51..48bf19b28979 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1126,6 +1126,11 @@ int Camera::queueRequest(Request *request) return -EXDEV; } + if (request->status() != Request::RequestPending) { + LOG(Camera, Error) << request->toString() << " is not prepared"; + return -EINVAL; + } + /* * The camera state may change until the end of the function. No locking * is however needed as PipelineHandler::queueRequest() will handle
Invalid, or not correctly reset requests can cause undefined behaviour in the pipeline handlers due to unexpected request state. If the status has not been reset to Request::RequestPending, it is either not new, or has not been correctly procesed through Request::reuse(). This can be caught early by validating the status of the request when it is queued to a camera. Reject invalid requests before processing them in the pipeline handlers. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera.cpp | 5 +++++ 1 file changed, 5 insertions(+)