| Message ID | 20221008111500.40661-1-Rauch.Christian@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain via libcamera-devel (2022-10-08 18:34:43) > Hi Christian > > On 10/8/22 4:45 PM, Christian Rauch via libcamera-devel wrote: > > Some devices may not be released via their 'PipelineHandler'. This results > > Can you explain better which are those devices / platform for which > PipelineHandler does not release the devices? Do you have any particular > case in mind? > > It's the job of the pipeline handler to correctly release the device, > since it's the one acquired it in the first place. > > in these devices still being acquired or busy on shutdown. Make sure that > > all devices are released when DeviceEnumerator is deconstructed. > > > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > --- > > src/libcamera/device_enumerator.cpp | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > > index f2e055de..d0968a0a 100644 > > --- a/src/libcamera/device_enumerator.cpp > > +++ b/src/libcamera/device_enumerator.cpp > > @@ -161,12 +161,8 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create() > > > > DeviceEnumerator::~DeviceEnumerator() > > { > > - for (const std::shared_ptr<MediaDevice> &media : devices_) { > > - if (media->busy()) > > - LOG(DeviceEnumerator, Error) > > - << "Removing media device " << media->deviceNode() > > - << " while still in use"; > > - } > > + for (const std::shared_ptr<MediaDevice> &media : devices_) > > + media->release(); > > I believe this shadows a bug and forcibly releases a device here, which > should ideally be released by the pipeline handler in the first place. > The Error log is present there to catch such instances I think! This is my worry too. I wonder if we can record who/where the media device is opened, and report that here? If this releases the media device, does that leave the pipeline handler still accessing it? (thus leading on to a new bug / use after free) -- Kieran > > } > > > > /** > > -- > > 2.34.1 > > >
Hi Christian On 10/8/22 4:45 PM, Christian Rauch via libcamera-devel wrote: > Some devices may not be released via their 'PipelineHandler'. This results Can you explain better which are those devices / platform for which PipelineHandler does not release the devices? Do you have any particular case in mind? It's the job of the pipeline handler to correctly release the device, since it's the one acquired it in the first place. > in these devices still being acquired or busy on shutdown. Make sure that > all devices are released when DeviceEnumerator is deconstructed. > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > --- > src/libcamera/device_enumerator.cpp | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > index f2e055de..d0968a0a 100644 > --- a/src/libcamera/device_enumerator.cpp > +++ b/src/libcamera/device_enumerator.cpp > @@ -161,12 +161,8 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create() > > DeviceEnumerator::~DeviceEnumerator() > { > - for (const std::shared_ptr<MediaDevice> &media : devices_) { > - if (media->busy()) > - LOG(DeviceEnumerator, Error) > - << "Removing media device " << media->deviceNode() > - << " while still in use"; > - } > + for (const std::shared_ptr<MediaDevice> &media : devices_) > + media->release(); I believe this shadows a bug and forcibly releases a device here, which should ideally be released by the pipeline handler in the first place. The Error log is present there to catch such instances I think! > } > > /** > -- > 2.34.1 >
Hi, For some reason, I did not receive the original response. Am 08.10.22 um 15:12 schrieb Kieran Bingham: > Quoting Umang Jain via libcamera-devel (2022-10-08 18:34:43) >> Hi Christian >> >> On 10/8/22 4:45 PM, Christian Rauch via libcamera-devel wrote: >>> Some devices may not be released via their 'PipelineHandler'. This results >> >> Can you explain better which are those devices / platform for which >> PipelineHandler does not release the devices? Do you have any particular >> case in mind? Specifically, it's https://bugs.libcamera.org/show_bug.cgi?id=155. >> >> It's the job of the pipeline handler to correctly release the device, >> since it's the one acquired it in the first place. The PipelineHandler does release the device when it is deconstructed, but the issue is here that not all PipelineHandler are deconstructed. Hence, devices are left acquired. >>> in these devices still being acquired or busy on shutdown. Make sure that >>> all devices are released when DeviceEnumerator is deconstructed. >>> >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>> --- >>> src/libcamera/device_enumerator.cpp | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp >>> index f2e055de..d0968a0a 100644 >>> --- a/src/libcamera/device_enumerator.cpp >>> +++ b/src/libcamera/device_enumerator.cpp >>> @@ -161,12 +161,8 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create() >>> >>> DeviceEnumerator::~DeviceEnumerator() >>> { >>> - for (const std::shared_ptr<MediaDevice> &media : devices_) { >>> - if (media->busy()) >>> - LOG(DeviceEnumerator, Error) >>> - << "Removing media device " << media->deviceNode() >>> - << " while still in use"; >>> - } >>> + for (const std::shared_ptr<MediaDevice> &media : devices_) >>> + media->release(); >> >> I believe this shadows a bug and forcibly releases a device here, which >> should ideally be released by the pipeline handler in the first place. >> The Error log is present there to catch such instances I think! > > This is my worry too. I wonder if we can record who/where the media > device is opened, and report that here? > > If this releases the media device, does that leave the pipeline handler > still accessing it? (thus leading on to a new bug / use after free) Are PipelineHandler expected to still access devices after the DeviceEnumerator has been deconstructed? > > -- > Kieran > > >>> } >>> >>> /** >>> -- >>> 2.34.1 >>> >>
Hi Christian, On 10/9/22 3:44 PM, Christian Rauch via libcamera-devel wrote: > Hi, > > For some reason, I did not receive the original response. > > Am 08.10.22 um 15:12 schrieb Kieran Bingham: >> Quoting Umang Jain via libcamera-devel (2022-10-08 18:34:43) >>> Hi Christian >>> >>> On 10/8/22 4:45 PM, Christian Rauch via libcamera-devel wrote: >>>> Some devices may not be released via their 'PipelineHandler'. This >>>> results >>> >>> Can you explain better which are those devices / platform for which >>> PipelineHandler does not release the devices? Do you have any >>> particular >>> case in mind? > > Specifically, it's https://bugs.libcamera.org/show_bug.cgi?id=155. The bug doesn't tell which platform is used (sensor + pipeline-handler). Can you provide addiitional information on the bug itself It would be good to inspect further using the debug logs. ($) LIBCAMERA_LOG_LEVELS=0 cam -c1 -C would be first step to know what's going on... > >>> >>> It's the job of the pipeline handler to correctly release the device, >>> since it's the one acquired it in the first place. > > The PipelineHandler does release the device when it is deconstructed, > but the issue is here that not all PipelineHandler are deconstructed. We need instances of such use-cases (if they are happening in the wild) I think DeviceEnumerator getting destroyed without getting PipelineHandler destroy - is quite ab-normal to me. > Hence, devices are left acquired. Maybe what your are experiencing is the crash before cleanup / destroy functions are invoked. Hence you see media devices are released etc stuff. But again, only can be ensured once we have additional information > >>>> in these devices still being acquired or busy on shutdown. Make >>>> sure that >>>> all devices are released when DeviceEnumerator is deconstructed. >>>> >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>>> --- >>>> src/libcamera/device_enumerator.cpp | 8 ++------ >>>> 1 file changed, 2 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/libcamera/device_enumerator.cpp >>>> b/src/libcamera/device_enumerator.cpp >>>> index f2e055de..d0968a0a 100644 >>>> --- a/src/libcamera/device_enumerator.cpp >>>> +++ b/src/libcamera/device_enumerator.cpp >>>> @@ -161,12 +161,8 @@ std::unique_ptr<DeviceEnumerator> >>>> DeviceEnumerator::create() >>>> >>>> DeviceEnumerator::~DeviceEnumerator() >>>> { >>>> - for (const std::shared_ptr<MediaDevice> &media : devices_) { >>>> - if (media->busy()) >>>> - LOG(DeviceEnumerator, Error) >>>> - << "Removing media device " << >>>> media->deviceNode() >>>> - << " while still in use"; >>>> - } >>>> + for (const std::shared_ptr<MediaDevice> &media : devices_) >>>> + media->release(); >>> >>> I believe this shadows a bug and forcibly releases a device here, >>> which >>> should ideally be released by the pipeline handler in the first place. >>> The Error log is present there to catch such instances I think! >> >> This is my worry too. I wonder if we can record who/where the media >> device is opened, and report that here? >> >> If this releases the media device, does that leave the pipeline handler >> still accessing it? (thus leading on to a new bug / use after free) > > Are PipelineHandler expected to still access devices after the > DeviceEnumerator has been deconstructed? > >> >> -- >> Kieran >> >> >>>> } >>>> >>>> /** >>>> -- >>>> 2.34.1 >>>> >>>
diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index f2e055de..d0968a0a 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -161,12 +161,8 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create() DeviceEnumerator::~DeviceEnumerator() { - for (const std::shared_ptr<MediaDevice> &media : devices_) { - if (media->busy()) - LOG(DeviceEnumerator, Error) - << "Removing media device " << media->deviceNode() - << " while still in use"; - } + for (const std::shared_ptr<MediaDevice> &media : devices_) + media->release(); } /**
Some devices may not be released via their 'PipelineHandler'. This results in these devices still being acquired or busy on shutdown. Make sure that all devices are released when DeviceEnumerator is deconstructed. Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> --- src/libcamera/device_enumerator.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) -- 2.34.1