[Spice-devel,spice-gtk,v1] channel-webdav: Do not crash if no PhodavServer is found

Submitted by Victor Toso on March 31, 2017, 8:35 a.m.

Details

Message ID 20170331083551.31523-1-victortoso@redhat.com
State New
Headers show
Series "channel-webdav: Do not crash if no PhodavServer is found" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso March 31, 2017, 8:35 a.m.
From: Victor Toso <me@victortoso.com>

This fixes a crash when spice_session_get_webdav_server() returns NULL
which can easily happen if no shared folder is set.

We still lack a way to tell the Guest about the failure as it waits
the data to mount the shared folder.

Log:
> (spicy:20825): phodav-CRITICAL **: phodav_server_get_soup_server: assertion
> 'PHODAV_IS_SERVER (self)' failed
> (spicy:20825): GSpice-DEBUG: channel-webdav.c:368 webdav-11:0: starting client
> -1812687888
>
> (spicy:20825): GLib-GObject-CRITICAL **: g_type_instance_get_private: assertion
> 'instance != NULL && instance->g_class != NULL' failed
>
> Thread 1 "spicy" received signal SIGSEGV, Segmentation fault.
> soup_server_accept_socket (server=server@entry=0x0, sock=sock@entry=0x7f7670
>
> [SoupSocket]) at soup-server.c:1468
> 1468            priv->clients = g_slist_prepend (priv->clients, client);

Backtrace:
> (gdb) bt
> #0  in soup_server_accept_socket (server=server@entry=0x0,
>     sock=sock@entry=0x7f7670[SoupSocket]) at soup-server.c:1468
> #1  in soup_server_accept_iostream (server=0x0,
>     stream=0xb77890 [GSimpleIOStream], local_addr=<optimized out>,
>     remote_addr=<optimized out>, error=<optimized out>) at soup-server.c:1515
> #2  in start_client (self=0xa0a6e0 [SpiceWebdavChannel]) at channel-webdav.c:382
> #3  in data_read_cb (source_object=0x9d2e60 [SpiceVmcInputStream],
>     res=0x9d0e80, user_data=0xa0a6e0) at channel-webdav.c:429
> #4  in g_task_return_now (task=0x9d0e80 [GTask]) at gtask.c:1121
> #5  in g_task_return (task=0x9d0e80 [GTask], type=<optimized out>) at gtask.c:1179
> #6  in complete_in_idle_cb (user_data=0x1233070) at vmcstream.c:110

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-webdav.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-webdav.c b/src/channel-webdav.c
index 4a246b5..38de2fb 100644
--- a/src/channel-webdav.c
+++ b/src/channel-webdav.c
@@ -361,9 +361,15 @@  static void start_client(SpiceWebdavChannel *self)
     SoupServer *server;
     GSocketAddress *addr;
     GError *error = NULL;
+    PhodavServer *phodav;
 
     session = spice_channel_get_session(SPICE_CHANNEL(self));
-    server = phodav_server_get_soup_server(spice_session_get_webdav_server(session));
+    phodav = spice_session_get_webdav_server(session);
+    if (phodav == NULL) {
+        CHANNEL_DEBUG(self, "failed to start client: no Phodav Server");
+        return;
+    }
+    server = phodav_server_get_soup_server(phodav);
 
     CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT, c->demux.client);
 

Comments

Hi

----- Original Message -----
> From: Victor Toso <me@victortoso.com>
> 
> This fixes a crash when spice_session_get_webdav_server() returns NULL
> which can easily happen if no shared folder is set.
> 

Can you start the phodav server without shared folder?

How did you reproduce?

> We still lack a way to tell the Guest about the failure as it waits
> the data to mount the shared folder.
> 
> Log:
> > (spicy:20825): phodav-CRITICAL **: phodav_server_get_soup_server: assertion
> > 'PHODAV_IS_SERVER (self)' failed
> > (spicy:20825): GSpice-DEBUG: channel-webdav.c:368 webdav-11:0: starting
> > client
> > -1812687888
> >
> > (spicy:20825): GLib-GObject-CRITICAL **: g_type_instance_get_private:
> > assertion
> > 'instance != NULL && instance->g_class != NULL' failed
> >
> > Thread 1 "spicy" received signal SIGSEGV, Segmentation fault.
> > soup_server_accept_socket (server=server@entry=0x0,
> > sock=sock@entry=0x7f7670
> >
> > [SoupSocket]) at soup-server.c:1468
> > 1468            priv->clients = g_slist_prepend (priv->clients, client);
> 
> Backtrace:

Hmm, looks like libsoup could use a g_return_if_fail

> > (gdb) bt
> > #0  in soup_server_accept_socket (server=server@entry=0x0,
> >     sock=sock@entry=0x7f7670[SoupSocket]) at soup-server.c:1468
> > #1  in soup_server_accept_iostream (server=0x0,
> >     stream=0xb77890 [GSimpleIOStream], local_addr=<optimized out>,
> >     remote_addr=<optimized out>, error=<optimized out>) at
> >     soup-server.c:1515
> > #2  in start_client (self=0xa0a6e0 [SpiceWebdavChannel]) at
> > channel-webdav.c:382
> > #3  in data_read_cb (source_object=0x9d2e60 [SpiceVmcInputStream],
> >     res=0x9d0e80, user_data=0xa0a6e0) at channel-webdav.c:429
> > #4  in g_task_return_now (task=0x9d0e80 [GTask]) at gtask.c:1121
> > #5  in g_task_return (task=0x9d0e80 [GTask], type=<optimized out>) at
> > gtask.c:1179
> > #6  in complete_in_idle_cb (user_data=0x1233070) at vmcstream.c:110
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/channel-webdav.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> index 4a246b5..38de2fb 100644
> --- a/src/channel-webdav.c
> +++ b/src/channel-webdav.c
> @@ -361,9 +361,15 @@ static void start_client(SpiceWebdavChannel *self)
>      SoupServer *server;
>      GSocketAddress *addr;
>      GError *error = NULL;
> +    PhodavServer *phodav;
>  
>      session = spice_channel_get_session(SPICE_CHANNEL(self));
> -    server =
> phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> +    phodav = spice_session_get_webdav_server(session);
> +    if (phodav == NULL) {
> +        CHANNEL_DEBUG(self, "failed to start client: no Phodav Server");
> +        return;
> +    }

g_return_if_fail() enough as well, there is already a clear DEBUG in spice-session.c, that seems redundant.


> +    server = phodav_server_get_soup_server(phodav);
>  
>      CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT,
>      c->demux.client);
>  
> --
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
----- Original Message -----
> Hi
> 
> ----- Original Message -----
> > From: Victor Toso <me@victortoso.com>
> > 
> > This fixes a crash when spice_session_get_webdav_server() returns NULL
> > which can easily happen if no shared folder is set.
> > 
> 
> Can you start the phodav server without shared folder?
> 
> How did you reproduce?

Oh that's the default (that changed over time iirc)

> 
> > We still lack a way to tell the Guest about the failure as it waits
> > the data to mount the shared folder.
> > 
> > Log:
> > > (spicy:20825): phodav-CRITICAL **: phodav_server_get_soup_server:
> > > assertion
> > > 'PHODAV_IS_SERVER (self)' failed
> > > (spicy:20825): GSpice-DEBUG: channel-webdav.c:368 webdav-11:0: starting
> > > client
> > > -1812687888
> > >
> > > (spicy:20825): GLib-GObject-CRITICAL **: g_type_instance_get_private:
> > > assertion
> > > 'instance != NULL && instance->g_class != NULL' failed
> > >
> > > Thread 1 "spicy" received signal SIGSEGV, Segmentation fault.
> > > soup_server_accept_socket (server=server@entry=0x0,
> > > sock=sock@entry=0x7f7670
> > >
> > > [SoupSocket]) at soup-server.c:1468
> > > 1468            priv->clients = g_slist_prepend (priv->clients, client);
> > 
> > Backtrace:
> 
> Hmm, looks like libsoup could use a g_return_if_fail
> 
> > > (gdb) bt
> > > #0  in soup_server_accept_socket (server=server@entry=0x0,
> > >     sock=sock@entry=0x7f7670[SoupSocket]) at soup-server.c:1468
> > > #1  in soup_server_accept_iostream (server=0x0,
> > >     stream=0xb77890 [GSimpleIOStream], local_addr=<optimized out>,
> > >     remote_addr=<optimized out>, error=<optimized out>) at
> > >     soup-server.c:1515
> > > #2  in start_client (self=0xa0a6e0 [SpiceWebdavChannel]) at
> > > channel-webdav.c:382
> > > #3  in data_read_cb (source_object=0x9d2e60 [SpiceVmcInputStream],
> > >     res=0x9d0e80, user_data=0xa0a6e0) at channel-webdav.c:429
> > > #4  in g_task_return_now (task=0x9d0e80 [GTask]) at gtask.c:1121
> > > #5  in g_task_return (task=0x9d0e80 [GTask], type=<optimized out>) at
> > > gtask.c:1179
> > > #6  in complete_in_idle_cb (user_data=0x1233070) at vmcstream.c:110
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/channel-webdav.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > index 4a246b5..38de2fb 100644
> > --- a/src/channel-webdav.c
> > +++ b/src/channel-webdav.c
> > @@ -361,9 +361,15 @@ static void start_client(SpiceWebdavChannel *self)
> >      SoupServer *server;
> >      GSocketAddress *addr;
> >      GError *error = NULL;
> > +    PhodavServer *phodav;
> >  
> >      session = spice_channel_get_session(SPICE_CHANNEL(self));
> > -    server =
> > phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> > +    phodav = spice_session_get_webdav_server(session);
> > +    if (phodav == NULL) {
> > +        CHANNEL_DEBUG(self, "failed to start client: no Phodav Server");
> > +        return;
> > +    }
> 
> g_return_if_fail() enough as well, there is already a clear DEBUG in
> spice-session.c, that seems redundant.
> 
> 
> > +    server = phodav_server_get_soup_server(phodav);
> >  
> >      CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT,
> >      c->demux.client);
> >  
> > --
> > 2.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
>
Hi

----- Original Message -----
> From: Victor Toso <me@victortoso.com>
> 
> This fixes a crash when spice_session_get_webdav_server() returns NULL
> which can easily happen if no shared folder is set.
> 
> We still lack a way to tell the Guest about the failure as it waits
> the data to mount the shared folder.
> 
> Log:
> > (spicy:20825): phodav-CRITICAL **: phodav_server_get_soup_server: assertion
> > 'PHODAV_IS_SERVER (self)' failed
> > (spicy:20825): GSpice-DEBUG: channel-webdav.c:368 webdav-11:0: starting
> > client
> > -1812687888
> >
> > (spicy:20825): GLib-GObject-CRITICAL **: g_type_instance_get_private:
> > assertion
> > 'instance != NULL && instance->g_class != NULL' failed
> >
> > Thread 1 "spicy" received signal SIGSEGV, Segmentation fault.
> > soup_server_accept_socket (server=server@entry=0x0,
> > sock=sock@entry=0x7f7670
> >
> > [SoupSocket]) at soup-server.c:1468
> > 1468            priv->clients = g_slist_prepend (priv->clients, client);
> 
> Backtrace:
> > (gdb) bt
> > #0  in soup_server_accept_socket (server=server@entry=0x0,
> >     sock=sock@entry=0x7f7670[SoupSocket]) at soup-server.c:1468
> > #1  in soup_server_accept_iostream (server=0x0,
> >     stream=0xb77890 [GSimpleIOStream], local_addr=<optimized out>,
> >     remote_addr=<optimized out>, error=<optimized out>) at
> >     soup-server.c:1515
> > #2  in start_client (self=0xa0a6e0 [SpiceWebdavChannel]) at
> > channel-webdav.c:382
> > #3  in data_read_cb (source_object=0x9d2e60 [SpiceVmcInputStream],
> >     res=0x9d0e80, user_data=0xa0a6e0) at channel-webdav.c:429
> > #4  in g_task_return_now (task=0x9d0e80 [GTask]) at gtask.c:1121
> > #5  in g_task_return (task=0x9d0e80 [GTask], type=<optimized out>) at
> > gtask.c:1179
> > #6  in complete_in_idle_cb (user_data=0x1233070) at vmcstream.c:110
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/channel-webdav.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> index 4a246b5..38de2fb 100644
> --- a/src/channel-webdav.c
> +++ b/src/channel-webdav.c
> @@ -361,9 +361,15 @@ static void start_client(SpiceWebdavChannel *self)
>      SoupServer *server;
>      GSocketAddress *addr;
>      GError *error = NULL;
> +    PhodavServer *phodav;
>  
>      session = spice_channel_get_session(SPICE_CHANNEL(self));
> -    server =
> phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> +    phodav = spice_session_get_webdav_server(session);
> +    if (phodav == NULL) {
> +        CHANNEL_DEBUG(self, "failed to start client: no Phodav Server");
> +        return;
> +    }

So it seems it's going to repeat this debug over whenever it reads data from the channel. I wonder if we should rather return an "empty" phodav server. This would avoid timeouts in the guest etc.. Drawback is that you can't change the shared dir after the server is started, but that's already a current limitation (I wonder if we have a RFE about it)

> +    server = phodav_server_get_soup_server(phodav);
>  
>      CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT,
>      c->demux.client);
>  
> --
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Hi,

On Mon, Apr 03, 2017 at 09:13:58AM -0400, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
> > From: Victor Toso <me@victortoso.com>
> >
> > This fixes a crash when spice_session_get_webdav_server() returns NULL
> > which can easily happen if no shared folder is set.
> >
>
> Can you start the phodav server without shared folder?
>
> How did you reproduce?

Running as root, with sudo ./tools/spicy ...

>
> > We still lack a way to tell the Guest about the failure as it waits
> > the data to mount the shared folder.
> > 
> > Log:
> > > (spicy:20825): phodav-CRITICAL **: phodav_server_get_soup_server: assertion
> > > 'PHODAV_IS_SERVER (self)' failed
> > > (spicy:20825): GSpice-DEBUG: channel-webdav.c:368 webdav-11:0: starting
> > > client
> > > -1812687888
> > >
> > > (spicy:20825): GLib-GObject-CRITICAL **: g_type_instance_get_private:
> > > assertion
> > > 'instance != NULL && instance->g_class != NULL' failed
> > >
> > > Thread 1 "spicy" received signal SIGSEGV, Segmentation fault.
> > > soup_server_accept_socket (server=server@entry=0x0,
> > > sock=sock@entry=0x7f7670
> > >
> > > [SoupSocket]) at soup-server.c:1468
> > > 1468            priv->clients = g_slist_prepend (priv->clients, client);
> > 
> > Backtrace:
>
> Hmm, looks like libsoup could use a g_return_if_fail

Yes!

> 
> > > (gdb) bt
> > > #0  in soup_server_accept_socket (server=server@entry=0x0,
> > >     sock=sock@entry=0x7f7670[SoupSocket]) at soup-server.c:1468
> > > #1  in soup_server_accept_iostream (server=0x0,
> > >     stream=0xb77890 [GSimpleIOStream], local_addr=<optimized out>,
> > >     remote_addr=<optimized out>, error=<optimized out>) at
> > >     soup-server.c:1515
> > > #2  in start_client (self=0xa0a6e0 [SpiceWebdavChannel]) at
> > > channel-webdav.c:382
> > > #3  in data_read_cb (source_object=0x9d2e60 [SpiceVmcInputStream],
> > >     res=0x9d0e80, user_data=0xa0a6e0) at channel-webdav.c:429
> > > #4  in g_task_return_now (task=0x9d0e80 [GTask]) at gtask.c:1121
> > > #5  in g_task_return (task=0x9d0e80 [GTask], type=<optimized out>) at
> > > gtask.c:1179
> > > #6  in complete_in_idle_cb (user_data=0x1233070) at vmcstream.c:110
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/channel-webdav.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > index 4a246b5..38de2fb 100644
> > --- a/src/channel-webdav.c
> > +++ b/src/channel-webdav.c
> > @@ -361,9 +361,15 @@ static void start_client(SpiceWebdavChannel *self)
> >      SoupServer *server;
> >      GSocketAddress *addr;
> >      GError *error = NULL;
> > +    PhodavServer *phodav;
> >  
> >      session = spice_channel_get_session(SPICE_CHANNEL(self));
> > -    server =
> > phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> > +    phodav = spice_session_get_webdav_server(session);
> > +    if (phodav == NULL) {
> > +        CHANNEL_DEBUG(self, "failed to start client: no Phodav Server");
> > +        return;
> > +    }
>
> g_return_if_fail() enough as well, there is already a clear DEBUG in
> spice-session.c, that seems redundant.

True, it is redundant. Logging a critical seems better indeed.

I'll send a followup patch soon.

>
> > +    server = phodav_server_get_soup_server(phodav);
> >  
> >      CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT,
> >      c->demux.client);
> >  
> > --
> > 2.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >