channel-webdav: avoid possible crash

Submitted by Victor Toso on Oct. 15, 2019, 9:58 a.m.

Details

Message ID 20191015095803.8559-1-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Oct. 15, 2019, 9:58 a.m.
From: Victor Toso <me@victortoso.com>

In case PhodavServer or SoupServer are NULL, we should stop and
return.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---

Rebased. Patch was related user report of crash in FlexVDI client [0].
Being careful in this not-hot path should be okay IMHO.

[0] https://lists.freedesktop.org/archives/spice-devel/2019-May/049070.html

 src/channel-webdav.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-webdav.c b/src/channel-webdav.c
index fb25084..de2843e 100644
--- a/src/channel-webdav.c
+++ b/src/channel-webdav.c
@@ -361,12 +361,17 @@  static void start_client(SpiceWebdavChannel *self)
     GIOStream *peer = NULL;
     SpiceSession *session;
     SoupServer *server;
+    PhodavServer* phodav_server;
     GSocketAddress *addr;
     GError *error = NULL;
     bool started;
 
     session = spice_channel_get_session(SPICE_CHANNEL(self));
-    server = phodav_server_get_soup_server(spice_session_get_webdav_server(session));
+    phodav_server = spice_session_get_webdav_server(session);
+    g_return_if_fail(phodav_server != NULL);
+
+    server = phodav_server_get_soup_server(phodav_server);
+    g_return_if_fail(server != NULL);
 
     CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT, c->demux.client);
 

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> In case PhodavServer or SoupServer are NULL, we should stop and
> return.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
> 
> Rebased. Patch was related user report of crash in FlexVDI client [0].
> Being careful in this not-hot path should be okay IMHO.
> 
> [0] https://lists.freedesktop.org/archives/spice-devel/2019-May/049070.html
> 
>  src/channel-webdav.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> index fb25084..de2843e 100644
> --- a/src/channel-webdav.c
> +++ b/src/channel-webdav.c
> @@ -361,12 +361,17 @@ static void start_client(SpiceWebdavChannel *self)
>      GIOStream *peer = NULL;
>      SpiceSession *session;
>      SoupServer *server;
> +    PhodavServer* phodav_server;
>      GSocketAddress *addr;
>      GError *error = NULL;
>      bool started;
>  
>      session = spice_channel_get_session(SPICE_CHANNEL(self));
> -    server =
> phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> +    phodav_server = spice_session_get_webdav_server(session);
> +    g_return_if_fail(phodav_server != NULL);
> +

As this will be always NULL we'll have a critical for each message sent by
the server.
g_return_if_fail documentation states:

"If expr evaluates to FALSE, the current function should be considered to have
undefined behaviour (a programmer error). The only correct solution to such
an error is to change the module that is calling the current function,
so that it avoids this incorrect call."

So this is a wrong usage of g_return_if_fail.

> +    server = phodav_server_get_soup_server(phodav_server);
> +    g_return_if_fail(server != NULL);
>  
>      CHANNEL_DEBUG(self, "starting client %" G_GINT64_FORMAT,
>      c->demux.client);
>  

Frediano