[Spice-devel,client,2/2,v2] streaming: Don't crash if the stream creation fails

Submitted by Francois Gouget on Aug. 8, 2016, 2:36 p.m.

Details

Message ID E1bWlfa-0007yq-6d@amboise
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Aug. 8, 2016, 2:36 p.m.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 src/channel-display.c | 53 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display.c b/src/channel-display.c
index b4c9ec0..f5dbf89 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -109,6 +109,7 @@  static display_surface *find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
 static void spice_display_channel_reset(SpiceChannel *channel, gboolean migrating);
 static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
 static void destroy_canvas(display_surface *surface);
+static void destroy_stream(SpiceChannel *channel, int id);
 static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer data);
 static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
 
@@ -1114,18 +1115,18 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
 #ifdef HAVE_BUILTIN_MJPEG
     case SPICE_VIDEO_CODEC_TYPE_MJPEG:
         st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
-        break;
+        return;
 #endif
     default:
 #ifdef HAVE_GSTVIDEO
         st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
+        return;
 #else
-        st->video_decoder = NULL;
+        break;
 #endif
     }
-    if (st->video_decoder == NULL) {
-        spice_printerr("could not create a video decoder for codec %u", op->codec_type);
-    }
+    spice_printerr("could not create a video decoder for codec %u", op->codec_type);
+    destroy_stream(channel, op->id);
 }
 
 static const SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
@@ -1231,6 +1232,7 @@  static void display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
     g_return_if_fail(c->nstreams > stream_id);
 
     st = channel->priv->streams[stream_id];
+    g_return_if_fail(st != NULL);
 
     if (!st->report_is_active) {
         return;
@@ -1353,6 +1355,7 @@  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
     g_return_if_fail(c->nstreams > op->id);
 
     st =  c->streams[op->id];
+    g_return_if_fail(st != NULL);
     mmtime = stream_get_time(st);
 
     if (spice_msg_in_type(in) == SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
@@ -1420,6 +1423,7 @@  static void display_handle_stream_clip(SpiceChannel *channel, SpiceMsgIn *in)
     g_return_if_fail(c->nstreams > op->id);
 
     st = c->streams[op->id];
+    g_return_if_fail(st != NULL);
 
     if (st->msg_clip) {
         spice_msg_in_unref(st->msg_clip);
@@ -1434,8 +1438,6 @@  static void destroy_stream(SpiceChannel *channel, int id)
 {
     SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     display_stream *st;
-    guint64 drops_duration_total = 0;
-    guint32 num_out_frames;
     int i;
 
     g_return_if_fail(c != NULL);
@@ -1446,20 +1448,22 @@  static void destroy_stream(SpiceChannel *channel, int id)
     if (!st)
         return;
 
-    num_out_frames = st->num_input_frames - st->arrive_late_count - st->num_drops_on_playback;
-    CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
-        "#drops-on-receive=%u avg-late-time(ms)=%.2f "
-        "#drops-on-playback=%u", __FUNCTION__,
-        id,
-        st->num_input_frames,
-        num_out_frames / (double)st->num_input_frames,
-        st->arrive_late_count,
-        st->arrive_late_count ? st->arrive_late_time / ((double)st->arrive_late_count): 0,
-        st->num_drops_on_playback);
-    if (st->num_drops_seqs) {
-        CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
-    }
-    for (i = 0; i < st->num_drops_seqs; i++) {
+    if (st->num_input_frames > 0) {
+        guint64 drops_duration_total = 0;
+        guint32 num_out_frames = st->num_input_frames - st->arrive_late_count - st->num_drops_on_playback;
+        CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
+            "#drops-on-receive=%u avg-late-time(ms)=%.2f "
+            "#drops-on-playback=%u", __FUNCTION__,
+            id,
+            st->num_input_frames,
+            num_out_frames / (double)st->num_input_frames,
+            st->arrive_late_count,
+            st->arrive_late_count ? st->arrive_late_time / ((double)st->arrive_late_count): 0,
+            st->num_drops_on_playback);
+        if (st->num_drops_seqs) {
+            CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
+        }
+        for (i = 0; i < st->num_drops_seqs; i++) {
             drops_sequence_stats *stats = &g_array_index(st->drops_seqs_stats_arr,
                                                          drops_sequence_stats,
                                                          i);
@@ -1468,9 +1472,10 @@  static void destroy_stream(SpiceChannel *channel, int id)
                                    stats->len,
                                    stats->start_mm_time - st->first_frame_mm_time,
                                    stats->duration);
-    }
-    if (st->num_drops_seqs) {
-        CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
+        }
+        if (st->num_drops_seqs) {
+            CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
+        }
     }
 
     g_array_free(st->drops_seqs_stats_arr, TRUE);

Comments

For reference the crash happened in display_handle_stream_data() when 
calling trying to queue the frame because video_decoder is NULL (same 
sort of issue as on the server with video_encoder).

    st->video_decoder->queue_frame(st->video_decoder, in, latency)

But if not there the call would happen in 
display_session_mm_time_reset_cb() when calling rescheduling frames:

        st->video_decoder->reschedule(st->video_decoder);

So it's simpler to just not create the stream object if we cannot create 
the corresponding video decoder.


Note that currently create_gstreamer_decoder() would only fail if 
gstvideo_init() fails but I'm looking at moving the create_pipeline() 
call there so it only returns non-NULL if that succeeded.
Hi,

Just replying here what we talked in IRC

On Mon, Aug 08, 2016 at 04:36:46PM +0200, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  src/channel-display.c | 53 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index b4c9ec0..f5dbf89 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -109,6 +109,7 @@ static display_surface *find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
>  static void spice_display_channel_reset(SpiceChannel *channel, gboolean migrating);
>  static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
>  static void destroy_canvas(display_surface *surface);
> +static void destroy_stream(SpiceChannel *channel, int id);
>  static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer data);
>  static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
>  
> @@ -1114,18 +1115,18 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>  #ifdef HAVE_BUILTIN_MJPEG
>      case SPICE_VIDEO_CODEC_TYPE_MJPEG:
>          st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
> -        break;
> +        return;
>  #endif
>      default:
>  #ifdef HAVE_GSTVIDEO
>          st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
> +        return;

As create_gstreamer_decoder() could fail (returning NULL) we should
check it and call destroy_stream() ...

>  #else
> -        st->video_decoder = NULL;
> +        break;
>  #endif
>      }
> -    if (st->video_decoder == NULL) {
> -        spice_printerr("could not create a video decoder for codec %u", op->codec_type);
> -    }
> +    spice_printerr("could not create a video decoder for codec %u", op->codec_type);
> +    destroy_stream(channel, op->id);

Maybe by keeping the if (st->video_decoder == NULL) and moving
destroy_stream() inside it would be enough.

Cheers,
  toso

>  }
>  
>  static const SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
> @@ -1231,6 +1232,7 @@ static void display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
>      g_return_if_fail(c->nstreams > stream_id);
>  
>      st = channel->priv->streams[stream_id];
> +    g_return_if_fail(st != NULL);
>  
>      if (!st->report_is_active) {
>          return;
> @@ -1353,6 +1355,7 @@ static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
>      g_return_if_fail(c->nstreams > op->id);
>  
>      st =  c->streams[op->id];
> +    g_return_if_fail(st != NULL);
>      mmtime = stream_get_time(st);
>  
>      if (spice_msg_in_type(in) == SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
> @@ -1420,6 +1423,7 @@ static void display_handle_stream_clip(SpiceChannel *channel, SpiceMsgIn *in)
>      g_return_if_fail(c->nstreams > op->id);
>  
>      st = c->streams[op->id];
> +    g_return_if_fail(st != NULL);
>  
>      if (st->msg_clip) {
>          spice_msg_in_unref(st->msg_clip);
> @@ -1434,8 +1438,6 @@ static void destroy_stream(SpiceChannel *channel, int id)
>  {
>      SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>      display_stream *st;
> -    guint64 drops_duration_total = 0;
> -    guint32 num_out_frames;
>      int i;
>  
>      g_return_if_fail(c != NULL);
> @@ -1446,20 +1448,22 @@ static void destroy_stream(SpiceChannel *channel, int id)
>      if (!st)
>          return;
>  
> -    num_out_frames = st->num_input_frames - st->arrive_late_count - st->num_drops_on_playback;
> -    CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
> -        "#drops-on-receive=%u avg-late-time(ms)=%.2f "
> -        "#drops-on-playback=%u", __FUNCTION__,
> -        id,
> -        st->num_input_frames,
> -        num_out_frames / (double)st->num_input_frames,
> -        st->arrive_late_count,
> -        st->arrive_late_count ? st->arrive_late_time / ((double)st->arrive_late_count): 0,
> -        st->num_drops_on_playback);
> -    if (st->num_drops_seqs) {
> -        CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
> -    }
> -    for (i = 0; i < st->num_drops_seqs; i++) {
> +    if (st->num_input_frames > 0) {
> +        guint64 drops_duration_total = 0;
> +        guint32 num_out_frames = st->num_input_frames - st->arrive_late_count - st->num_drops_on_playback;
> +        CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
> +            "#drops-on-receive=%u avg-late-time(ms)=%.2f "
> +            "#drops-on-playback=%u", __FUNCTION__,
> +            id,
> +            st->num_input_frames,
> +            num_out_frames / (double)st->num_input_frames,
> +            st->arrive_late_count,
> +            st->arrive_late_count ? st->arrive_late_time / ((double)st->arrive_late_count): 0,
> +            st->num_drops_on_playback);
> +        if (st->num_drops_seqs) {
> +            CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
> +        }
> +        for (i = 0; i < st->num_drops_seqs; i++) {
>              drops_sequence_stats *stats = &g_array_index(st->drops_seqs_stats_arr,
>                                                           drops_sequence_stats,
>                                                           i);
> @@ -1468,9 +1472,10 @@ static void destroy_stream(SpiceChannel *channel, int id)
>                                     stats->len,
>                                     stats->start_mm_time - st->first_frame_mm_time,
>                                     stats->duration);
> -    }
> -    if (st->num_drops_seqs) {
> -        CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
> +        }
> +        if (st->num_drops_seqs) {
> +            CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
> +        }
>      }
>  
>      g_array_free(st->drops_seqs_stats_arr, TRUE);
> -- 
> 2.8.1
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, Aug 08, 2016 at 06:00:23PM +0200, Francois Gouget wrote:
> 
> For reference the crash happened in display_handle_stream_data() when 
> calling trying to queue the frame because video_decoder is NULL (same 
> sort of issue as on the server with video_encoder).
> 
>     st->video_decoder->queue_frame(st->video_decoder, in, latency)
> 
> But if not there the call would happen in 
> display_session_mm_time_reset_cb() when calling rescheduling frames:
> 
>         st->video_decoder->reschedule(st->video_decoder);
> 
> So it's simpler to just not create the stream object if we cannot create 
> the corresponding video decoder.
> 
> 
> Note that currently create_gstreamer_decoder() would only fail if 
> gstvideo_init() fails but I'm looking at moving the create_pipeline() 
> call there so it only returns non-NULL if that succeeded.


I'd add that to the commit log rather than having the rationale/bug
description buried in the mailing list archives.

Christophe