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

[Steve Lhomme]

On 19/02/2019 08:11, Rémi Denis-Courmont wrote:
> 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.

It's probably true in many cases. But I don't think this is the case 
here. We should use the right tools for the right task.

>
> 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é.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel