[vlc-devel] [vlc-commits] decoder: add Destroy helper function for decoder owners

[Rémi Denis-Courmont]

Memory debugging is not a specific tool. There were plenty of tools doing it before ASan, many of which are still available. And most likely there will be better alternatives or newer versions of ASan in the future.

Besides, ASan is neither OS-specific nor architecture-specific.

And finally, yes, defensive programming does make things much harder to debug since it hides bugs. Exactly what Thomas explained already.

Le 19 février 2019 08:48:16 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>My point is that relying on specific tools for a specific OS as the
>sole solution for code safety have proved insufficient so many times.
>
>As for the double free issue that would be harder to debug, it's a non
>issue since double free would never occur with my solution. Why would
>you need tools to debug it then?
>
>> On 18 Feb 2019, at 17:38, Rémi Denis-Courmont <remi at remlab.net>
>wrote:
>> 
>> Le maanantaina 18. helmikuuta 2019, 17.52.32 EET Steve Lhomme a écrit
>:
>>> On 18/02/2019 16:32, Thomas Guillem wrote:
>>>> On Mon, Feb 18, 2019, at 16:11, Steve Lhomme wrote:
>>>>> vlc | branch: master | Steve Lhomme <robux4 at ycbcr.xyz> | Mon Feb
>18
>>>>> 14:50:21 2019 +0100| [4a79e6a4929085cd78024e0c0367c2bc01ab11b4] |
>>>>> committer: Steve Lhomme
>>>>> 
>>>>> decoder: add Destroy helper function for decoder owners
>>>>> 
>>>>>>
>http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=4a79e6a4929085cd7
>>>>>> 8024e0c0367c2bc01ab11b4>> 
>>>>> ---
>>>>> 
>>>>>  include/vlc_codec.h         |  7 +++++++
>>>>>  src/input/decoder_helpers.c | 29 +++++++++++++++++++++++++++++
>>>>>  src/libvlccore.sym          |  1 +
>>>>>  3 files changed, 37 insertions(+)
>>>>> 
>>>>> diff --git a/include/vlc_codec.h b/include/vlc_codec.h
>>>>> index c287953ba7..2042ad2424 100644
>>>>> --- a/include/vlc_codec.h
>>>>> +++ b/include/vlc_codec.h
>>>>> @@ -320,6 +320,13 @@ VLC_API void decoder_AbortPictures( decoder_t
>>>>> *dec, bool b_abort );
>>>>> 
>>>>>  VLC_API void decoder_Init( decoder_t *dec, const es_format_t * );
>>>>> 
>>>>>  /**
>>>>> 
>>>>> + * Destroy a decoder and reset the structure.
>>>>> + *
>>>>> + * To be used by decoder owners.
>>>>> + */
>>>>> +VLC_API void decoder_Destroy( decoder_t **p_dec );
>>>>> +
>>>>> +/**
>>>>> 
>>>>>   * This function queues a single picture to the video output.
>>>>>   *
>>>>>   * \note
>>>>> 
>>>>> diff --git a/src/input/decoder_helpers.c
>b/src/input/decoder_helpers.c
>>>>> index 51f7ce8f6d..b60b830123 100644
>>>>> --- a/src/input/decoder_helpers.c
>>>>> +++ b/src/input/decoder_helpers.c
>>>>> @@ -28,6 +28,8 @@
>>>>> 
>>>>>  #include <vlc_common.h>
>>>>>  #include <vlc_codec.h>
>>>>> 
>>>>> +#include <vlc_meta.h>
>>>>> +#include <vlc_modules.h>
>>>>> 
>>>>>  void decoder_Init( decoder_t *p_dec, const es_format_t *restrict
>p_fmt
>>>>> 
>>>>> )
>>>>> 
>>>>>  {
>>>>> 
>>>>> @@ -43,3 +45,30 @@ void decoder_Init( decoder_t *p_dec, const
>>>>> es_format_t *restrict p_fmt )
>>>>> 
>>>>>      es_format_Copy( &p_dec->fmt_in, p_fmt );
>>>>>      es_format_Init( &p_dec->fmt_out, p_fmt->i_cat, 0 );
>>>>> 
>>>>>  }
>>>>> 
>>>>> +
>>>>> +static void decoder_Clean( decoder_t *p_dec )
>>>>> +{
>>>>> +    es_format_Clean( &p_dec->fmt_in );
>>>>> +    es_format_Clean( &p_dec->fmt_out );
>>>>> +
>>>>> +    if ( p_dec->p_description )
>>>>> +    {
>>>>> +        vlc_meta_Delete(p_dec->p_description);
>>>>> +        p_dec->p_description = NULL;
>>>>> +    }
>>>>> +    if ( p_dec->p_module != NULL )
>>>>> +    {
>>>>> +        module_unneed(p_dec, p_dec->p_module);
>>>>> +        p_dec->p_module = NULL;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +void decoder_Destroy( decoder_t **p_dec )
>>>>> +{
>>>>> +    if (*p_dec != NULL)
>>>>> +    {
>>>>> +        decoder_Clean( *p_dec );
>>>>> +        vlc_object_release( *p_dec );
>>>>> +        *p_dec = NULL;
>>>>> +    }
>>>> 
>>>> I really prefer to use a decoder_t * here.
>>>> There is not many functions in VLC that set the pointer to NULL.
>This will
>>>> add more confusion.
>>>> 
>>>> I really prefer having a crash (via assert/ASAN) when a client is
>misusing
>>>> an API (calling Destroy with a freed pointer) instead of ignoring
>it.
>> 
>>> A double free doesn't always crash on Windows and there's no ASAN.
>> 
>> So your point is that because the bug is hard to observe on Windows,
>it should 
>> be made hard to observe everywhere? This is punishing everybody for
>your 
>> personal selection of development environment. What the hell...
>> 
>> -- 
>> 雷米‧德尼-库尔蒙
>> http://www.remlab.net/
>> 
>> 
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190219/03d16a2c/attachment-0001.html>