[vlc-devel] [PATCH] input: var: fix #18332: multiple bookmark options leading to UB

[Filip Roséen]

The crash was due to the previous imlpementation inaccurate
assumptions regarding the input-data, more specifically it assumed
that only one bookmarks= option would be present (something which is
not guaranteed to be true).

These changes also fixes a premature release of the input_item_t's
mutex.
---
 src/input/var.c | 57 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/src/input/var.c b/src/input/var.c
index 9882b9e60a..43c50fd2e2 100644
--- a/src/input/var.c
+++ b/src/input/var.c
@@ -172,12 +172,13 @@ static const char *GetEsVarName( enum es_format_category_e i_cat )
 static void UpdateBookmarksOption( input_thread_t *p_input )
 {
     input_thread_private_t *priv = input_priv(p_input);
+    input_item_t* item = priv->p_item;
     struct vlc_memstream vstr;
 
     vlc_memstream_open( &vstr );
     vlc_memstream_puts( &vstr, "bookmarks=" );
 
-    vlc_mutex_lock( &priv->p_item->lock );
+    vlc_mutex_lock( &item->lock );
     var_Change( p_input, "bookmark", VLC_VAR_CLEARCHOICES );
 
     for( int i = 0; i < priv->i_bookmark; i++ )
@@ -194,31 +195,49 @@ static void UpdateBookmarksOption( input_thread_t *p_input )
             i > 0 ? "," : "", sp->psz_name, ( 1. * sp->i_time_offset ) / CLOCK_FREQ );
     }
 
-    vlc_mutex_unlock( &priv->p_item->lock );
-
-    if( vlc_memstream_close( &vstr ) == VLC_SUCCESS )
+    if( vlc_memstream_close( &vstr ) )
     {
-        bool b_overwritten = false;
+        vlc_mutex_unlock( &item->lock );
+        return;
+    }
 
-        for( int i = 0; i < priv->p_item->i_options; ++i )
-        {
-            char** ppsz_option = &priv->p_item->ppsz_options[i];
+    /* XXX: The below is ugly and should be fixed elsewhere, but in order to
+     * not have more than one "bookmarks=" option associated with the item, we
+     * need to remove any existing ones before adding the new one. This logic
+     * should exist in input_item_AddOption with "OPTION_UNIQUE & <an overwrite
+     * flag>, but until then we handle it here. */
 
-            if( strncmp( *ppsz_option, "bookmarks=", 10 ) == 0 )
-            {
-                free( *ppsz_option );
-                *ppsz_option = vstr.ptr;
-                b_overwritten = true;
-            }
-        }
+    char** const orig_beg = &item->ppsz_options[0];
+    char** const orig_end = orig_beg + item->i_options;
+    char** end = orig_end;
 
-        if( !b_overwritten )
+    for( char** option = orig_beg; option != end; )
+    {
+        if( strncmp( *option, "bookmarks=", 10 ) )
+            ++option;
+        else
         {
-            input_item_AddOption( priv->p_item, vstr.ptr,
-                                  VLC_INPUT_OPTION_UNIQUE );
-            free( vstr.ptr );
+            free( *option );
+            /* It might be tempting to optimize the below by overwriting
+             * *option with the value of the last element, however; we want to
+             * preserve the order of the other options (as behavior might
+             * depend on it) */
+            memmove( option, option + 1, ( --end - option ) * sizeof *end );
         }
     }
+
+    if( end != orig_end ) /* we removed at least 1 option */
+    {
+        *end = vstr.ptr;
+        item->i_options = end - orig_beg + 1;
+        vlc_mutex_unlock( &priv->p_item->lock );
+    }
+    else /* nothing removed, add the usual way */
+    {
+        vlc_mutex_unlock( &priv->p_item->lock );
+        input_item_AddOption( item, vstr.ptr, VLC_INPUT_OPTION_UNIQUE );
+        free( vstr.ptr );
+    }
 }
 
 void input_LegacyEvents( input_thread_t *p_input, void *user_data,
-- 
2.18.0