[vlc-devel] [PATCH] input: Don't probe the stream when controlling a demux_Filter
Rémi Denis-Courmont
remi at remlab.net
Thu Jan 11 06:13:40 CET 2018
Le torstaina 11. tammikuuta 2018, 6.38.12 EET Rémi Denis-Courmont a écrit :
> Le 10 janvier 2018 19:06:31 GMT+02:00, "Hugo Beauzée-Luyssen"
<hugo at beauzee.fr> a écrit :
> >When a stream filter is active, attempting to read any file which
> >demux doesn't implement DEMUX_CAN_PAUSE, DEMUX_CAN_CONTROL_PACE,
> >DEMUX_GET_PTS_DELAY or DEMUX_SET_PAUSE_STATE will cause undefined
> >behavior when accessing demux_t::s
> >---
> >src/input/demux.c | 139
> >++++++++++++++++++++++++++++++------------------------
> >
> > 1 file changed, 77 insertions(+), 62 deletions(-)
> >
> >diff --git a/src/input/demux.c b/src/input/demux.c
> >index 18b64b5f31..e594014ac0 100644
> >--- a/src/input/demux.c
> >+++ b/src/input/demux.c
> >@@ -155,6 +155,7 @@ typedef struct demux_priv_t
> >
> > {
> >
> > demux_t demux;
> > void (*destroy)(demux_t *);
> >
> >+ int (*control)( demux_t *, int, va_list );
> >
> > } demux_priv_t;
> >
> > static void demux_DestroyDemux(demux_t *demux)
> >
> >@@ -191,6 +192,80 @@ static int demux_Probe(void *func, va_list ap)
> >
> > return probe(VLC_OBJECT(demux));
> >
> > }
> >
> >+static int demux_ControlInternal( demux_t *demux, int query, ... )
> >+{
> >+ int ret;
> >+ va_list ap;
> >+
> >+ va_start( ap, query );
> >+ ret = demux->pf_control( demux, query, ap );
> >+ va_end( ap );
> >+ return ret;
> >+}
> >+
> >+int demux_vaControl( demux_t *demux, int query, va_list args )
> >+{
> >+ demux_priv_t *priv = (demux_priv_t *)demux;
> >+ return priv->control( demux, query, args );
> >+}
> >+
> >+static int demux_vaControlInner( demux_t *demux, int query, va_list
> >args )
> >+{
> >+ if( demux->s != NULL )
> >+ switch( query )
> >+ {
> >+ /* Legacy fallback for missing getters in synchronous
> >demuxers */
> >+ case DEMUX_CAN_PAUSE:
> >+ case DEMUX_CAN_CONTROL_PACE:
> >+ case DEMUX_GET_PTS_DELAY:
> >+ {
> >+ int ret;
> >+ va_list ap;
> >+
> >+ va_copy( ap, args );
> >+ ret = demux->pf_control( demux, query, args );
> >+ if( ret != VLC_SUCCESS )
> >+ ret = vlc_stream_vaControl( demux->s, query, ap );
> >+ va_end( ap );
> >+ return ret;
> >+ }
> >+
> >+ /* Some demuxers need to control pause directly (e.g.
> >adaptive),
> >+ * but many legacy demuxers do not understand pause at
> >all.
> >+ * If DEMUX_CAN_PAUSE is not implemented, bypass the
> >demuxer and
> >+ * byte stream. If DEMUX_CAN_PAUSE is implemented and
> >pause is
> >+ * supported, pause the demuxer normally. Else, something
> >went very
> >+ * wrong.
> >+ *
> >+ * Note that this requires asynchronous/threaded demuxers
> >to
> >+ * always return VLC_SUCCESS for DEMUX_CAN_PAUSE, so that
> >they are
> >+ * never bypassed. Otherwise, we would reenter demux->s
> >callbacks
> >+ * and break thread safety. At the time of writing,
> >asynchronous or
> >+ * threaded *non-access* demuxers do not exist and are not
> >fully
> >+ * supported by the input thread, so this is theoretical.
> >*/
> >+ case DEMUX_SET_PAUSE_STATE:
> >+ {
> >+ bool can_pause;
> >+
> >+ if( demux_ControlInternal( demux, DEMUX_CAN_PAUSE,
> >+ &can_pause ) )
> >+ return vlc_stream_vaControl( demux->s, query, args
> >);
> >+
> >+ /* The caller shall not pause if pause is unsupported.
> >*/
> >+ assert( can_pause );
> >+ break;
> >+ }
> >+ }
> >+
> >+ return demux->pf_control( demux, query, args );
> >+}
> >+
> >+static int demux_vaControlFilter( demux_t *demux, int query, va_list
> >args )
> >+{
> >+ // Do not probe the stream in the context of a demux filter.
> >+ return demux->pf_control( demux, query, args );
> >+}
> >+
> >/**************************************************************************
> >***>
> > * demux_NewAdvanced:
> > * if s is NULL then load a access_demux
> >
> >@@ -245,6 +320,7 @@ demux_t *demux_NewAdvanced( vlc_object_t *p_obj,
> >input_thread_t *p_parent_input,
> >
> > p_demux->p_sys = NULL;
> > p_demux->info.i_update = 0;
> > priv->destroy = s ? demux_DestroyDemux : demux_DestroyAccessDemux;
> >
> >+ priv->control = demux_vaControlInner;
> >
> > if( s != NULL )
> > {
> >
> >@@ -303,68 +379,6 @@ void demux_Delete( demux_t *p_demux )
> >
> > #define static_control_match(foo) \
> >
> > static_assert((unsigned) DEMUX_##foo == STREAM_##foo, "Mismatch")
> >
> >-static int demux_ControlInternal( demux_t *demux, int query, ... )
> >-{
> >- int ret;
> >- va_list ap;
> >-
> >- va_start( ap, query );
> >- ret = demux->pf_control( demux, query, ap );
> >- va_end( ap );
> >- return ret;
> >-}
> >-
> >-int demux_vaControl( demux_t *demux, int query, va_list args )
> >-{
> >- if( demux->s != NULL )
> >- switch( query )
> >- {
> >- /* Legacy fallback for missing getters in synchronous
> >demuxers */
> >- case DEMUX_CAN_PAUSE:
> >- case DEMUX_CAN_CONTROL_PACE:
> >- case DEMUX_GET_PTS_DELAY:
> >- {
> >- int ret;
> >- va_list ap;
> >-
> >- va_copy( ap, args );
> >- ret = demux->pf_control( demux, query, args );
> >- if( ret != VLC_SUCCESS )
> >- ret = vlc_stream_vaControl( demux->s, query, ap );
> >- va_end( ap );
> >- return ret;
> >- }
> >-
> >- /* Some demuxers need to control pause directly (e.g.
> >adaptive),
> >- * but many legacy demuxers do not understand pause at
> >all.
> >- * If DEMUX_CAN_PAUSE is not implemented, bypass the
> >demuxer and
> >- * byte stream. If DEMUX_CAN_PAUSE is implemented and
> >pause is
> >- * supported, pause the demuxer normally. Else, something
> >went very
> >- * wrong.
> >- *
> >- * Note that this requires asynchronous/threaded demuxers
> >to
> >- * always return VLC_SUCCESS for DEMUX_CAN_PAUSE, so that
> >they are
> >- * never bypassed. Otherwise, we would reenter demux->s
> >callbacks
> >- * and break thread safety. At the time of writing,
> >asynchronous or
> >- * threaded *non-access* demuxers do not exist and are not
> >fully
> >- * supported by the input thread, so this is theoretical.
> >*/
> >- case DEMUX_SET_PAUSE_STATE:
> >- {
> >- bool can_pause;
> >-
> >- if( demux_ControlInternal( demux, DEMUX_CAN_PAUSE,
> >- &can_pause ) )
> >- return vlc_stream_vaControl( demux->s, query, args
> >);
> >-
> >- /* The caller shall not pause if pause is unsupported.
> >*/
> >- assert( can_pause );
> >- break;
> >- }
> >- }
> >-
> >- return demux->pf_control( demux, query, args );
> >-}
> >-
> >/**************************************************************************
> >***>
> > * demux_vaControlHelper:
> >***************************************************************************
> >**/ @@ -593,6 +607,7 @@ static demux_t *demux_FilterNew( demux_t *p_next,
> >const char *p_name )
> >
> > p_demux->psz_filepath = NULL;
> > p_demux->out = NULL;
> > priv->destroy = demux_DestroyDemuxFilter;
> >
> >+ priv->control = demux_vaControlFilter;
> >
> > p_demux->p_module =
> >
> > module_need( p_demux, "demux_filter", p_name, p_name != NULL );
>
> This is UB even with the patch, only much more needlessly complicated.
To be clear:
Without the patch, a demux_t gets misused as a stream_t. With the patch
DEMUX_GET_PTS_DELAY and DEMUX_CAN_PAUSE can return VLC_EGENERIC to the input.
Both scenarii are UB.
Also the way it handles access_demux is rather odd.
--
雷米‧德尼-库尔蒙
https://www.remlab.net/
More information about the vlc-devel
mailing list