SEGFAULT in obexd manager.c:find_session

SEGFAULT in obexd manager.c:find_session

[Marcin Zawiejski]

Hi, I think there is a bug in obexd in manager.c:find_session function.

What happens here is a segfault when manager.c:find_session calls 
g_str_equal(obc_session_get_path(session), path). This is caused by the 
sessions list having a session with a NULL path.

Basically when I call manager.c:create_session, the session created 
there is added to sessions list but it has a NULL path until the 
manager.c:create_callback is called.

However the manager.c:create_callback is not called at all if the remote 
device refuses the connection. So when manager.c:find_session is called, 
it actually calls the g_str_equal(NULL, path) causing the segfault.

This might be simply fixed by modifying the manager.c:find_session to 
check for a NULL session path before calling g_str_equal(...).

The problem is reproducible by having two sessions, with one awaiting 
connection and another one with an active file transfer. When the file 
transfer errors and I call org.bluez.obex.Client1 RemoveSession then the 
obexd segfaults since the session awaiting connection has a NULL path.

I'm not sure if the session with a NULL path should be on the sessions 
list or not. If its okay, then here's a simple patch for the 
manager.c:find_session function:

---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..28b890c 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -142,8 +142,9 @@ static struct obc_session *find_session(const char 
*path)

         for (l = sessions; l; l = l->next) {
                 struct obc_session *session = l->data;
+               const char *session_path = obc_session_get_path(session);

-               if (g_str_equal(obc_session_get_path(session), path) == 
TRUE)
+               if (session_path != NULL && g_str_equal(session_path, 
path) == TRUE)
                         return session;
         }
---

Marcin.

Re: SEGFAULT in obexd manager.c:find_session

[Luiz Augusto von Dentz]

Hi Marcin,

On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> Hi, I think there is a bug in obexd in manager.c:find_session function.
>
> What happens here is a segfault when manager.c:find_session calls
> g_str_equal(obc_session_get_path(session), path). This is caused by the
> sessions list having a session with a NULL path.
>
> Basically when I call manager.c:create_session, the session created there is
> added to sessions list but it has a NULL path until the
> manager.c:create_callback is called.
>
> However the manager.c:create_callback is not called at all if the remote
> device refuses the connection. So when manager.c:find_session is called, it
> actually calls the g_str_equal(NULL, path) causing the segfault.
>
> This might be simply fixed by modifying the manager.c:find_session to check
> for a NULL session path before calling g_str_equal(...).
>
> The problem is reproducible by having two sessions, with one awaiting
> connection and another one with an active file transfer. When the file
> transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> obexd segfaults since the session awaiting connection has a NULL path.
>
> I'm not sure if the session with a NULL path should be on the sessions list
> or not. If its okay, then here's a simple patch for the
> manager.c:find_session function:
>
> ---
> diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> index 8f62a30..28b890c 100644
> --- a/obexd/client/manager.c
> +++ b/obexd/client/manager.c
> @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> *path)
>
>         for (l = sessions; l; l = l->next) {
>                 struct obc_session *session = l->data;
> +               const char *session_path = obc_session_get_path(session);
>
> -               if (g_str_equal(obc_session_get_path(session), path) ==
> TRUE)
> +               if (session_path != NULL && g_str_equal(session_path, path)
> == TRUE)
>                         return session;
>         }
> ---

You can use g_strcmp0 which checks for NULL, gonna take a look why the
session path is NULL perhaps we should not even add to the session
list until the connection completes and it is properly registered.


--
Luiz Augusto von Dentz

Re: SEGFAULT in obexd manager.c:find_session

[Marcin Zawiejski]

Hi Luiz,

On Sun, 2012-12-30 at 16:28 +0200, Luiz Augusto von Dentz wrote:
> Hi Marcin,
> 
> On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> > Hi, I think there is a bug in obexd in manager.c:find_session function.
> >
> > What happens here is a segfault when manager.c:find_session calls
> > g_str_equal(obc_session_get_path(session), path). This is caused by the
> > sessions list having a session with a NULL path.
> >
> > Basically when I call manager.c:create_session, the session created there is
> > added to sessions list but it has a NULL path until the
> > manager.c:create_callback is called.
> >
> > However the manager.c:create_callback is not called at all if the remote
> > device refuses the connection. So when manager.c:find_session is called, it
> > actually calls the g_str_equal(NULL, path) causing the segfault.
> >
> > This might be simply fixed by modifying the manager.c:find_session to check
> > for a NULL session path before calling g_str_equal(...).
> >
> > The problem is reproducible by having two sessions, with one awaiting
> > connection and another one with an active file transfer. When the file
> > transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> > obexd segfaults since the session awaiting connection has a NULL path.
> >
> > I'm not sure if the session with a NULL path should be on the sessions list
> > or not. If its okay, then here's a simple patch for the
> > manager.c:find_session function:
> >
> > ---
> > diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> > index 8f62a30..28b890c 100644
> > --- a/obexd/client/manager.c
> > +++ b/obexd/client/manager.c
> > @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> > *path)
> >
> >         for (l = sessions; l; l = l->next) {
> >                 struct obc_session *session = l->data;
> > +               const char *session_path = obc_session_get_path(session);
> >
> > -               if (g_str_equal(obc_session_get_path(session), path) ==
> > TRUE)
> > +               if (session_path != NULL && g_str_equal(session_path, path)
> > == TRUE)
> >                         return session;
> >         }
> > ---
> 
> You can use g_strcmp0 which checks for NULL, gonna take a look why the
> session path is NULL perhaps we should not even add to the session
> list until the connection completes and it is properly registered.

I think you are right - session with no path should not be placed on the
sessions list because it is not usable in such case. Below is a patch
that works for me, here's a summary of the changes:

- move g_slist_append from create_session to create_callback so the
session is added to sessions list after it has been registered
- split shutdown_session into shutdown_session and release_session; the
shutdown_session performs the shutdown and unref on the session but does
not try to remove session from the list; the release_session removes the
session from the list and calls release_shutdown
- modify create_callback so it calls shutdown_session for failures
before the session has been registered (so it doesn't try to remove
session from the list since the session is not on the list)
- modify remove_session to call release_session in order to remove the
session from list and do shutdown

---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..118dd48 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
 
 static void shutdown_session(struct obc_session *session)
 {
-	sessions = g_slist_remove(sessions, session);
 	obc_session_shutdown(session);
 	obc_session_unref(session);
 }
 
+static void release_session(struct obc_session *session)
+{
+	sessions = g_slist_remove(sessions, session);
+	shutdown_session(session);
+}
+
 static void unregister_session(void *data)
 {
 	struct obc_session *session = data;
@@ -94,6 +99,16 @@ static void create_callback(struct obc_session
*session,
 
 	path = obc_session_register(session, unregister_session);
 
+	if(path == NULL) {
+		DBusMessage *error = g_dbus_create_error(data->message,
+					ERROR_INTERFACE ".Failed",
+					NULL);
+		g_dbus_send_message(data->connection, error);
+		shutdown_session(session);
+		goto done;
+	}
+
+	sessions = g_slist_append(sessions, session);
 	g_dbus_send_reply(data->connection, data->message,
 				DBUS_TYPE_OBJECT_PATH, &path,
 				DBUS_TYPE_INVALID);
@@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
*connection,
 					dbus_message_get_sender(message),
 					create_callback, data);
 	if (session != NULL) {
-		sessions = g_slist_append(sessions, session);
 		return NULL;
 	}
 
@@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
*connection,
 				ERROR_INTERFACE ".NotAuthorized",
 				"Not Authorized");
 
-	shutdown_session(session);
+	release_session(session);
 
 	return dbus_message_new_method_return(message);
 }
---

Marcin.

Re: SEGFAULT in obexd manager.c:find_session

[Marcin Zawiejski]

Hi Luiz,

On Sun, 2012-12-30 at 16:28 +0200, Luiz Augusto von Dentz wrote:
> Hi Marcin,
> 
> On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> > Hi, I think there is a bug in obexd in manager.c:find_session function.
> >
> > What happens here is a segfault when manager.c:find_session calls
> > g_str_equal(obc_session_get_path(session), path). This is caused by the
> > sessions list having a session with a NULL path.
> >
> > Basically when I call manager.c:create_session, the session created there is
> > added to sessions list but it has a NULL path until the
> > manager.c:create_callback is called.
> >
> > However the manager.c:create_callback is not called at all if the remote
> > device refuses the connection. So when manager.c:find_session is called, it
> > actually calls the g_str_equal(NULL, path) causing the segfault.
> >
> > This might be simply fixed by modifying the manager.c:find_session to check
> > for a NULL session path before calling g_str_equal(...).
> >
> > The problem is reproducible by having two sessions, with one awaiting
> > connection and another one with an active file transfer. When the file
> > transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> > obexd segfaults since the session awaiting connection has a NULL path.
> >
> > I'm not sure if the session with a NULL path should be on the sessions list
> > or not. If its okay, then here's a simple patch for the
> > manager.c:find_session function:
> >
> > ---
> > diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> > index 8f62a30..28b890c 100644
> > --- a/obexd/client/manager.c
> > +++ b/obexd/client/manager.c
> > @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> > *path)
> >
> >         for (l = sessions; l; l = l->next) {
> >                 struct obc_session *session = l->data;
> > +               const char *session_path = obc_session_get_path(session);
> >
> > -               if (g_str_equal(obc_session_get_path(session), path) ==
> > TRUE)
> > +               if (session_path != NULL && g_str_equal(session_path, path)
> > == TRUE)
> >                         return session;
> >         }
> > ---
> 
> You can use g_strcmp0 which checks for NULL, gonna take a look why the
> session path is NULL perhaps we should not even add to the session
> list until the connection completes and it is properly registered.

I think you are right - session with no path should not be placed on the
sessions list because it is not usable in such case anyway. Below is a patch
that works for me, here's a summary of the changes:

- move g_slist_append from create_session to create_callback so the
session is added to sessions list after it has been registered
- split shutdown_session into shutdown_session and release_session; the
shutdown_session performs the shutdown and unref on the session but does
not try to remove session from the list; the release_session removes the
session from the list and calls release_shutdown
- modify create_callback so it calls shutdown_session for failures
before the session has been registered (so it doesn't try to remove
session from the list since the session is not on the list)
- modify remove_session to call release_session in order to remove the
session from list and do shutdown

---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..118dd48 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
 
 static void shutdown_session(struct obc_session *session)
 {
-	sessions = g_slist_remove(sessions, session);
 	obc_session_shutdown(session);
 	obc_session_unref(session);
 }
 
+static void release_session(struct obc_session *session)
+{
+	sessions = g_slist_remove(sessions, session);
+	shutdown_session(session);
+}
+
 static void unregister_session(void *data)
 {
 	struct obc_session *session = data;
@@ -94,6 +99,16 @@ static void create_callback(struct obc_session
*session,
 
 	path = obc_session_register(session, unregister_session);
 
+	if(path == NULL) {
+		DBusMessage *error = g_dbus_create_error(data->message,
+					ERROR_INTERFACE ".Failed",
+					NULL);
+		g_dbus_send_message(data->connection, error);
+		shutdown_session(session);
+		goto done;
+	}
+
+	sessions = g_slist_append(sessions, session);
 	g_dbus_send_reply(data->connection, data->message,
 				DBUS_TYPE_OBJECT_PATH, &path,
 				DBUS_TYPE_INVALID);
@@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
*connection,
 					dbus_message_get_sender(message),
 					create_callback, data);
 	if (session != NULL) {
-		sessions = g_slist_append(sessions, session);
 		return NULL;
 	}
 
@@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
*connection,
 				ERROR_INTERFACE ".NotAuthorized",
 				"Not Authorized");
 
-	shutdown_session(session);
+	release_session(session);
 
 	return dbus_message_new_method_return(message);
 }
---

Marcin.

Re: SEGFAULT in obexd manager.c:find_session

[Luiz Augusto von Dentz]

Hi Marcin,

On Mon, Dec 31, 2012 at 1:58 AM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> ---
> diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> index 8f62a30..118dd48 100644
> --- a/obexd/client/manager.c
> +++ b/obexd/client/manager.c
> @@ -59,11 +59,16 @@ static GSList *sessions = NULL;
>
>  static void shutdown_session(struct obc_session *session)
>  {
> -       sessions = g_slist_remove(sessions, session);
>         obc_session_shutdown(session);
>         obc_session_unref(session);
>  }
>
> +static void release_session(struct obc_session *session)
> +{
> +       sessions = g_slist_remove(sessions, session);
> +       shutdown_session(session);
> +}
> +
>  static void unregister_session(void *data)
>  {
>         struct obc_session *session = data;
> @@ -94,6 +99,16 @@ static void create_callback(struct obc_session
> *session,
>
>         path = obc_session_register(session, unregister_session);

Normally we don't have empty lines between assigning and if statements
checking the values, also please follow the code style e.g.
if<space>(<condition>) it is quite similar to the one use in kernel.

> +       if(path == NULL) {
> +               DBusMessage *error = g_dbus_create_error(data->message,
> +                                       ERROR_INTERFACE ".Failed",
> +                                       NULL);
> +               g_dbus_send_message(data->connection, error);
> +               shutdown_session(session);
> +               goto done;
> +       }
> +
> +       sessions = g_slist_append(sessions, session);
>         g_dbus_send_reply(data->connection, data->message,
>                                 DBUS_TYPE_OBJECT_PATH, &path,
>                                 DBUS_TYPE_INVALID);
> @@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
> *connection,
>                                         dbus_message_get_sender(message),
>                                         create_callback, data);
>         if (session != NULL) {
> -               sessions = g_slist_append(sessions, session);
>                 return NULL;
>         }
>
> @@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
> *connection,
>                                 ERROR_INTERFACE ".NotAuthorized",
>                                 "Not Authorized");
>
> -       shutdown_session(session);
> +       release_session(session);
>
>         return dbus_message_new_method_return(message);
>  }
> ---

Besides that the patch looks good, once you fix the code style please
send this a proper patch e.g. git format-patch + git send-email and
please include the backtrace in the patch description.


--
Luiz Augusto von Dentz