[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