SEGFAULT in obexd manager.c:find_session

PATCH Marcin Zawiejski 2 participants 5 messages Dec 30, 2012 – Dec 31, 2012 Bluetooth drivers

Share & tools

Marcin Zawiejski

author

2012-12-30 13:18 UTC #0 lore raw

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:

---

obexd/client/manager.c +2 -2

index 8f62a30..28b890c 100644
@@ -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.

Luiz Augusto von Dentz

maintainer

2012-12-30 14:28 UTC #1 lore raw

Hi Marcin,

Quoted 44 lines · 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:
---
obexd/client/manager.c +2 -1
index 8f62a30..28b890c 100644
@@ -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;
        }

Quoted 1 line

---

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

Marcin Zawiejski

author

2012-12-30 23:58 UTC #2 lore raw

Hi Luiz,

Quoted 41 lines · Luiz Augusto von Dentz

Hi Marcin,
Quoted 38 lines · 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:
---
obexd/client/manager.c +1 -0
index 8f62a30..28b890c 100644
@@ -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);
 

Quoted 11 lines

Quoted 7 lines
obexd/client/manager.c +1 -1
-               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

---

obexd/client/manager.c +6 -1

index 8f62a30..118dd48 100644
@@ -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.

Luiz Augusto von Dentz

maintainer

2012-12-31 10:13 UTC #3 lore raw

Hi Marcin,

Quoted 24 lines · Marcin Zawiejski

---

obexd/client/manager.c +6 -1

index 8f62a30..118dd48 100644
@@ -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;

obexd/client/manager.c static void create_callback(struct obc_session · line 101

Quoted 3 lines

obexd/client/manager.c +0 -0

@@ -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.

Quoted 31 lines

obexd/client/manager.c +11 -2

+       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);

Quoted 2 lines

}
---

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

Marcin Zawiejski

author

2012-12-31 00:01 UTC #4 lore raw

In reply to Luiz Augusto von Dentz

Hi Luiz,

Quoted 41 lines · Luiz Augusto von Dentz

Hi Marcin,
Quoted 38 lines · 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:
---
obexd/client/manager.c +1 -0
index 8f62a30..28b890c 100644
@@ -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);
 

Quoted 11 lines

Quoted 7 lines
obexd/client/manager.c +1 -1
-               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:

---

obexd/client/manager.c +6 -1

index 8f62a30..118dd48 100644
@@ -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);

@@ -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.