hlsdemux: Fix accessing invalidated memory

In gst_hls_demux_get_next_fragment() the next fragment URI gets
stored in next_fragment_uri, but the gst_hls_demux_updates_loop()
can at any time update the playlist, rendering this string invalid.
Therefore, any data (like key, iv, URIs) that is taken from a
GstM3U8Client needs to be copied. In addition, accessing the
internals of a GstM3U8Client requires locking.

https://bugzilla.gnome.org/show_bug.cgi?id=737793

diff --git a/ext/hls/gsthlsdemux.c b/ext/hls/gsthlsdemux.c
old mode 100644 (file)
new mode 100755 (executable)
index e32858b..68144c1
--- a/ext/hls/gsthlsdemux.c
+++ b/ext/hls/gsthlsdemux.c
@@ -588,7 +588,7 @@ gst_hls_demux_sink_event (GstPad * pad, GstObject * parent, GstEvent * event)
 
   switch (event->type) {
     case GST_EVENT_EOS:{
-      gchar *playlist = NULL;
+      gchar *uri, *playlist = NULL;
 
       if (demux->playlist == NULL) {
         GST_WARNING_OBJECT (demux, "Received EOS without a playlist.");
@@ -602,7 +602,7 @@ gst_hls_demux_sink_event (GstPad * pad, GstObject * parent, GstEvent * event)
       ret = gst_pad_peer_query (demux->sinkpad, query);
       if (ret) {
         gboolean permanent;
-        gchar *uri, *redirect_uri;
+        gchar *redirect_uri;
 
         gst_query_parse_uri (query, &uri);
         gst_query_parse_uri_redirection (query, &redirect_uri);
@@ -618,16 +618,17 @@ gst_hls_demux_sink_event (GstPad * pad, GstObject * parent, GstEvent * event)
       }
       gst_query_unref (query);
 
+      uri = gst_m3u8_client_get_uri (demux->client);
       gst_element_post_message (GST_ELEMENT_CAST (demux),
           gst_message_new_element (GST_OBJECT_CAST (demux),
               gst_structure_new (STATISTICS_MESSAGE_NAME,
-                  "manifest-uri", G_TYPE_STRING,
-                  gst_m3u8_client_get_uri (demux->client), "uri", G_TYPE_STRING,
-                  gst_m3u8_client_get_uri (demux->client),
+                  "manifest-uri", G_TYPE_STRING, uri,
+                  "uri", G_TYPE_STRING, uri,
                   "manifest-download-start", GST_TYPE_CLOCK_TIME,
                   GST_CLOCK_TIME_NONE,
                   "manifest-download-stop", GST_TYPE_CLOCK_TIME,
                   gst_util_get_timestamp (), NULL)));
+      g_free (uri);
 
       playlist = gst_hls_src_buf_to_utf8_playlist (demux->playlist);
       demux->playlist = NULL;
@@ -694,9 +695,14 @@ gst_hls_demux_src_query (GstPad * pad, GstObject * parent, GstQuery * query)
     }
     case GST_QUERY_URI:
       if (hlsdemux->client) {
+        gchar *uri;
+
         /* FIXME: Do we answer with the variant playlist, with the current
          * playlist or the the uri of the least downlowaded fragment? */
-        gst_query_set_uri (query, gst_m3u8_client_get_uri (hlsdemux->client));
+        uri = gst_m3u8_client_get_uri (hlsdemux->client);
+        gst_query_set_uri (query, uri);
+        g_free (uri);
+
         ret = TRUE;
       }
       break;
@@ -1405,8 +1411,14 @@ gst_hls_demux_reset (GstHLSDemux * demux, gboolean dispose)
   if (demux->pending_buffer)
     gst_buffer_unref (demux->pending_buffer);
   demux->pending_buffer = NULL;
-  demux->current_key = NULL;
-  demux->current_iv = NULL;
+  if (demux->current_key) {
+    g_free (demux->current_key);
+    demux->current_key = NULL;
+  }
+  if (demux->current_iv) {
+    g_free (demux->current_iv);
+    demux->current_iv = NULL;
+  }
   gst_hls_demux_decrypt_end (demux);
 
   demux->current_download_rate = -1;
@@ -1575,25 +1587,28 @@ gst_hls_demux_update_playlist (GstHLSDemux * demux, gboolean update,
   GstBuffer *buf;
   gchar *playlist;
   gboolean main_checked = FALSE, updated = FALSE;
-  const gchar *uri;
+  gchar *uri, *main_uri;
 
 retry:
   uri = gst_m3u8_client_get_current_uri (demux->client);
+  main_uri = gst_m3u8_client_get_uri (demux->client);
   download =
       gst_uri_downloader_fetch_uri (demux->downloader, uri,
-      demux->client->main ? demux->client->main->uri : NULL, TRUE, TRUE, TRUE,
-      err);
+      main_uri, TRUE, TRUE, TRUE, err);
+  g_free (main_uri);
   if (download == NULL) {
     if (update && !main_checked
         && gst_m3u8_client_has_variant_playlist (demux->client)
-        && demux->client->main) {
+        && gst_m3u8_client_has_main (demux->client)) {
       GError *err2 = NULL;
+      main_uri = gst_m3u8_client_get_uri (demux->client);
       GST_INFO_OBJECT (demux,
           "Updating playlist %s failed, attempt to refresh variant playlist %s",
-          uri, demux->client->main->uri);
+          uri, main_uri);
       download =
           gst_uri_downloader_fetch_uri (demux->downloader,
-          demux->client->main->uri, NULL, TRUE, TRUE, TRUE, &err2);
+          main_uri, NULL, TRUE, TRUE, TRUE, &err2);
+      g_free (main_uri);
       g_clear_error (&err2);
       if (download != NULL) {
         gchar *base_uri;
@@ -1607,6 +1622,7 @@ retry:
           return FALSE;
         }
 
+        g_free (uri);
         if (download->redirect_permanent && download->redirect_uri) {
           uri = download->redirect_uri;
           base_uri = NULL;
@@ -1627,13 +1643,17 @@ retry:
         main_checked = TRUE;
         goto retry;
       } else {
+        g_free (uri);
         return FALSE;
       }
     } else {
+      g_free (uri);
       return FALSE;
     }
   }
 
+  g_free (uri);
+
   /* Set the base URI of the playlist to the redirect target if any */
   GST_M3U8_CLIENT_LOCK (demux->client);
   g_free (demux->client->current->uri);
@@ -1667,26 +1687,30 @@ retry:
     return FALSE;
   }
 
+  uri = gst_m3u8_client_get_current_uri(demux->client);
+  main_uri = gst_m3u8_client_get_uri (demux->client);
   gst_element_post_message (GST_ELEMENT_CAST (demux),
       gst_message_new_element (GST_OBJECT_CAST (demux),
           gst_structure_new (STATISTICS_MESSAGE_NAME,
-              "manifest-uri", G_TYPE_STRING,
-              gst_m3u8_client_get_uri (demux->client), "uri", G_TYPE_STRING,
-              gst_m3u8_client_get_current_uri (demux->client),
+              "manifest-uri", G_TYPE_STRING, main_uri,
+              "uri", G_TYPE_STRING, uri,
               "manifest-download-start", GST_TYPE_CLOCK_TIME,
               download->download_start_time,
               "manifest-download-stop", GST_TYPE_CLOCK_TIME,
               download->download_stop_time, NULL)));
+  g_free (main_uri);
+  g_free (uri);
 
   g_object_unref (download);
 
+  GST_M3U8_CLIENT_LOCK (demux->client);
+
   /* If it's a live source, do not let the sequence number go beyond
    * three fragments before the end of the list */
   if (update == FALSE && demux->client->current &&
-      gst_m3u8_client_is_live (demux->client)) {
+      GST_M3U8_CLIENT_IS_LIVE (demux->client)) {
     gint64 last_sequence;
 
-    GST_M3U8_CLIENT_LOCK (demux->client);
     last_sequence =
         GST_M3U8_MEDIA_FILE (g_list_last (demux->client->current->
             files)->data)->sequence;
@@ -1697,8 +1721,7 @@ retry:
       demux->need_segment = TRUE;
       demux->client->sequence = last_sequence - 3;
     }
-    GST_M3U8_CLIENT_UNLOCK (demux->client);
-  } else if (demux->client->current && !gst_m3u8_client_is_live (demux->client)) {
+  } else if (demux->client->current && !GST_M3U8_CLIENT_IS_LIVE (demux->client)) {
     GstClockTime current_pos, target_pos;
     guint sequence = 0;
     GList *walk;
@@ -1707,7 +1730,6 @@ retry:
      * playlists, so get the correct fragment here based on the current
      * position
      */
-    GST_M3U8_CLIENT_LOCK (demux->client);
     current_pos = 0;
     target_pos = demux->segment.position;
     for (walk = demux->client->current->files; walk; walk = walk->next) {
@@ -1725,9 +1747,10 @@ retry:
       sequence++;
     demux->client->sequence = sequence;
     demux->client->sequence_position = current_pos;
-    GST_M3U8_CLIENT_UNLOCK (demux->client);
   }
 
+  GST_M3U8_CLIENT_UNLOCK (demux->client);
+
   return updated;
 }
 
@@ -1769,13 +1792,17 @@ retry_failover_protection:
   demux->new_playlist = TRUE;
 
   if (gst_hls_demux_update_playlist (demux, FALSE, NULL)) {
+    gchar *uri, *main_uri;
+    uri = gst_m3u8_client_get_current_uri (demux->client);
+    main_uri = gst_m3u8_client_get_uri (demux->client);
     gst_element_post_message (GST_ELEMENT_CAST (demux),
         gst_message_new_element (GST_OBJECT_CAST (demux),
             gst_structure_new (STATISTICS_MESSAGE_NAME,
-                "manifest-uri", G_TYPE_STRING,
-                gst_m3u8_client_get_uri (demux->client), "uri", G_TYPE_STRING,
-                gst_m3u8_client_get_current_uri (demux->client), "bitrate",
-                G_TYPE_INT, new_bandwidth, NULL)));
+                "manifest-uri", G_TYPE_STRING, main_uri,
+                "uri", G_TYPE_STRING, uri,
+                "bitrate", G_TYPE_INT, new_bandwidth, NULL)));
+    g_free (uri);
+    g_free (main_uri);
   } else {
     GList *failover = NULL;
 
@@ -2090,13 +2117,13 @@ static gboolean
 gst_hls_demux_get_next_fragment (GstHLSDemux * demux,
     gboolean * end_of_playlist, GError ** err)
 {
-  const gchar *next_fragment_uri;
+  gchar *uri, *next_fragment_uri;
   GstClockTime duration;
   GstClockTime timestamp;
   gboolean discont;
   gint64 range_start, range_end;
-  const gchar *key = NULL;
-  const guint8 *iv = NULL;
+  gchar *key = NULL;
+  guint8 *iv = NULL;
   GstClockTime download_start_time = 0;
   GstFlowReturn ret = GST_FLOW_OK;
 
@@ -2120,7 +2147,11 @@ gst_hls_demux_get_next_fragment (GstHLSDemux * demux,
   demux->current_duration = duration;
   demux->starting_fragment = TRUE;
   demux->reset_crypto = TRUE;
+  if (demux->current_key)
+    g_free (demux->current_key);
   demux->current_key = key;
+  if (demux->current_iv)
+    g_free (demux->current_iv);
   demux->current_iv = iv;
 
   /* Reset last flow return */
@@ -2129,15 +2160,19 @@ gst_hls_demux_get_next_fragment (GstHLSDemux * demux,
   g_clear_error (&demux->last_error);
   g_mutex_unlock (&demux->fragment_download_lock);
 
+  GST_M3U8_CLIENT_LOCK (demux->client);
   if (!gst_hls_demux_update_source (demux, next_fragment_uri,
           demux->client->main ? demux->client->main->uri : NULL,
           FALSE,
           demux->client->current ? demux->client->current->allowcache : TRUE)) {
+    GST_M3U8_CLIENT_UNLOCK (demux->client);
     *err =
         g_error_new (GST_CORE_ERROR, GST_CORE_ERROR_MISSING_PLUGIN,
         "Missing plugin to handle URI: '%s'", next_fragment_uri);
+    g_free (next_fragment_uri);
     return FALSE;
   }
+  GST_M3U8_CLIENT_UNLOCK (demux->client);
 
   gst_hls_demux_configure_src_pad (demux);
 
@@ -2155,6 +2190,7 @@ gst_hls_demux_get_next_fragment (GstHLSDemux * demux,
             "Source element can't handle range requests");
         demux->last_ret = GST_FLOW_ERROR;
         g_mutex_unlock (&demux->fragment_download_lock);
+        g_free (next_fragment_uri);
         return FALSE;
       }
     }
@@ -2207,23 +2243,26 @@ gst_hls_demux_get_next_fragment (GstHLSDemux * demux,
     if (demux->segment.rate > 0)
       demux->segment.position += demux->current_duration;
 
+    uri = gst_m3u8_client_get_uri (demux->client);
     gst_element_post_message (GST_ELEMENT_CAST (demux),
         gst_message_new_element (GST_OBJECT_CAST (demux),
             gst_structure_new (STATISTICS_MESSAGE_NAME,
-                "manifest-uri", G_TYPE_STRING,
-                gst_m3u8_client_get_uri (demux->client), "uri", G_TYPE_STRING,
-                next_fragment_uri, "fragment-start-time",
-                GST_TYPE_CLOCK_TIME, download_start_time,
+                "manifest-uri", G_TYPE_STRING, uri,
+                "uri", G_TYPE_STRING, next_fragment_uri,
+                "fragment-start-time", GST_TYPE_CLOCK_TIME, download_start_time,
                 "fragment-stop-time", GST_TYPE_CLOCK_TIME,
                 gst_util_get_timestamp (), "fragment-size", G_TYPE_UINT64,
                 demux->download_total_bytes, "fragment-download-time",
                 GST_TYPE_CLOCK_TIME, demux->download_total_time * GST_USECOND,
                 NULL)));
+    g_free (uri);
   }
 
   /* clear the ghostpad eos state */
   gst_hls_demux_stream_clear_eos_state (demux);
 
+  g_free (next_fragment_uri);
+
   if (ret != GST_FLOW_OK)
     return FALSE;
 

diff --git a/ext/hls/gsthlsdemux.h b/ext/hls/gsthlsdemux.h
old mode 100644 (file)
new mode 100755 (executable)
index 8241df1..4e790f6
--- a/ext/hls/gsthlsdemux.h
+++ b/ext/hls/gsthlsdemux.h
@@ -139,8 +139,8 @@ struct _GstHLSDemux
 #else
   gcry_cipher_hd_t aes_ctx;
 #endif
-  const gchar *current_key;
-  const guint8 *current_iv;
+  gchar *current_key;
+  guint8 *current_iv;
   GstAdapter *adapter; /* used to accumulate 16 bytes multiple chunks */
   GstBuffer *pending_buffer; /* decryption scenario:
                               * the last buffer can only be pushed when

diff --git a/ext/hls/m3u8.c b/ext/hls/m3u8.c
old mode 100644 (file)
new mode 100755 (executable)
index 7fa9699..c737b3d
--- a/ext/hls/m3u8.c
+++ b/ext/hls/m3u8.c
@@ -932,9 +932,9 @@ find_next_fragment (GstM3U8Client * client, GList * l, gboolean forward)
 
 gboolean
 gst_m3u8_client_get_next_fragment (GstM3U8Client * client,
-    gboolean * discontinuity, const gchar ** uri, GstClockTime * duration,
+    gboolean * discontinuity, gchar ** uri, GstClockTime * duration,
     GstClockTime * timestamp, gint64 * range_start, gint64 * range_end,
-    const gchar ** key, const guint8 ** iv, gboolean forward)
+    gchar ** key, guint8 ** iv, gboolean forward)
 {
   GList *l;
   GstM3U8MediaFile *file;
@@ -964,7 +964,7 @@ gst_m3u8_client_get_next_fragment (GstM3U8Client * client,
   if (discontinuity)
     *discontinuity = client->sequence != file->sequence || file->discont;
   if (uri)
-    *uri = file->uri;
+    *uri = g_strdup (file->uri);
   if (duration)
     *duration = file->duration;
   if (range_start)
@@ -972,9 +972,11 @@ gst_m3u8_client_get_next_fragment (GstM3U8Client * client,
   if (range_end)
     *range_end = file->size != -1 ? file->offset + file->size - 1 : -1;
   if (key)
-    *key = file->key;
-  if (iv)
-    *iv = file->iv;
+    *key = g_strdup (file->key);
+  if (iv) {
+    *iv = g_new (guint8, sizeof (file->iv));
+    memcpy (*iv, file->iv, sizeof (file->iv));
+  }
 
   client->sequence = file->sequence;
 
@@ -1054,33 +1056,49 @@ gst_m3u8_client_get_target_duration (GstM3U8Client * client)
   return duration;
 }
 
-const gchar *
+gchar *
 gst_m3u8_client_get_uri (GstM3U8Client * client)
 {
-  const gchar *uri;
+  gchar *uri;
 
   g_return_val_if_fail (client != NULL, NULL);
 
   GST_M3U8_CLIENT_LOCK (client);
-  uri = client->main->uri;
+  uri = client->main ? g_strdup (client->main->uri) : NULL;
   GST_M3U8_CLIENT_UNLOCK (client);
   return uri;
 }
 
-const gchar *
+gchar *
 gst_m3u8_client_get_current_uri (GstM3U8Client * client)
 {
-  const gchar *uri;
+  gchar *uri;
 
   g_return_val_if_fail (client != NULL, NULL);
 
   GST_M3U8_CLIENT_LOCK (client);
-  uri = client->current->uri;
+  uri = g_strdup (client->current->uri);
   GST_M3U8_CLIENT_UNLOCK (client);
   return uri;
 }
 
 gboolean
+gst_m3u8_client_has_main(GstM3U8Client * client)
+{
+  gboolean ret;
+
+  g_return_val_if_fail (client != NULL, FALSE);
+
+  GST_M3U8_CLIENT_LOCK (client);
+  if (client->main)
+    ret = TRUE;
+  else
+    ret = FALSE;
+  GST_M3U8_CLIENT_UNLOCK (client);
+  return ret;
+}
+
+gboolean
 gst_m3u8_client_has_variant_playlist (GstM3U8Client * client)
 {
   gboolean ret;
@@ -1101,10 +1119,7 @@ gst_m3u8_client_is_live (GstM3U8Client * client)
   g_return_val_if_fail (client != NULL, FALSE);
 
   GST_M3U8_CLIENT_LOCK (client);
-  if (!client->current || client->current->endlist)
-    ret = FALSE;
-  else
-    ret = TRUE;
+  ret = GST_M3U8_CLIENT_IS_LIVE (client);
   GST_M3U8_CLIENT_UNLOCK (client);
   return ret;
 }

diff --git a/ext/hls/m3u8.h b/ext/hls/m3u8.h
old mode 100644 (file)
new mode 100755 (executable)
index 2344b22..77b42d5
--- a/ext/hls/m3u8.h
+++ b/ext/hls/m3u8.h
@@ -35,6 +35,8 @@ typedef struct _GstM3U8Client GstM3U8Client;
 #define GST_M3U8_CLIENT_LOCK(c) g_mutex_lock (&c->lock);
 #define GST_M3U8_CLIENT_UNLOCK(c) g_mutex_unlock (&c->lock);
 
+#define GST_M3U8_CLIENT_IS_LIVE(c) ((!(c)->current || (c)->current->endlist) ? FALSE : TRUE)
+
 struct _GstM3U8
 {
   gchar *uri;                   /* actually downloaded URI */
@@ -94,14 +96,15 @@ gboolean gst_m3u8_client_update (GstM3U8Client * client, gchar * data);
 gboolean gst_m3u8_client_update_variant_playlist (GstM3U8Client * client, gchar * data, const gchar * uri, const gchar * base_uri);
 void gst_m3u8_client_set_current (GstM3U8Client * client, GstM3U8 * m3u8);
 gboolean gst_m3u8_client_get_next_fragment (GstM3U8Client * client,
-    gboolean * discontinuity, const gchar ** uri, GstClockTime * duration,
+    gboolean * discontinuity, gchar ** uri, GstClockTime * duration,
     GstClockTime * timestamp, gint64 * range_start, gint64 * range_end,
-    const gchar ** key, const guint8 ** iv, gboolean forward);
+    gchar ** key, guint8 ** iv, gboolean forward);
 void gst_m3u8_client_advance_fragment (GstM3U8Client * client, gboolean forward);
 GstClockTime gst_m3u8_client_get_duration (GstM3U8Client * client);
 GstClockTime gst_m3u8_client_get_target_duration (GstM3U8Client * client);
-const gchar *gst_m3u8_client_get_uri(GstM3U8Client * client);
-const gchar *gst_m3u8_client_get_current_uri(GstM3U8Client * client);
+gchar *gst_m3u8_client_get_uri(GstM3U8Client * client);
+gchar *gst_m3u8_client_get_current_uri(GstM3U8Client * client);
+gboolean gst_m3u8_client_has_main(GstM3U8Client * client);
 gboolean gst_m3u8_client_has_variant_playlist(GstM3U8Client * client);
 gboolean gst_m3u8_client_is_live(GstM3U8Client * client);
 GList * gst_m3u8_client_get_playlist_for_bitrate (GstM3U8Client * client,