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

[Umang Jain]

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