[libcamera-devel] [PATCH] libcamera: Release all devices on shutdown

[Christian Rauch]

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 at 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
>>>
>>