[PATCH BlueZ 0/6] Improve around advertise of bluetoothctl

[PATCH BlueZ 0/6] Improve around advertise of bluetoothctl

[ERAMOTO Masaya]

This patch set fixes bugs of advertising.c and refactors
advertise-related code.

ERAMOTO Masaya (6):
  client: Fix memory leak of advertise command
  client: Fix core dump when using set-advertise-name
  client: Prevent to pass invalid type to D-Bus
  client: Use existing function for parsing argument
  client: Introduce parse_argument()
  client: Use new parse_argument() function

 client/advertising.c |  14 +++++--
 client/main.c        | 107 ++++++++++++---------------------------------------
 2 files changed, 35 insertions(+), 86 deletions(-)

-- 
2.7.4

[PATCH BlueZ 1/6] client: Fix memory leak of advertise command

[ERAMOTO Masaya]

Since advertise command does not free type when repeating to enable and
disable advertising, the following memory leak occurs.

  11 bytes in 1 blocks are definitely lost in loss record 20 of 190
     at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
     by 0x4E89718: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
     by 0x4EA24EE: g_strdup (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
     by 0x40EBC8: ad_register (advertising.c:343)
     by 0x40A666: cmd_advertise (main.c:2344)
     by 0x40ABA3: rl_handler (main.c:2664)
     by 0x53C16F4: rl_callback_read_char (in /lib/x86_64-linux-gnu/libreadline.so.6.3)
     by 0x405AFC: input_handler (main.c:110)
     by 0x4E84049: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
     by 0x4E843EF: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
     by 0x4E84711: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
     by 0x4055FE: main (main.c:2865)
---
 client/advertising.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/client/advertising.c b/client/advertising.c
index 72c4ccb..b105da9 100644
--- a/client/advertising.c
+++ b/client/advertising.c
@@ -340,6 +340,7 @@ void ad_register(DBusConnection *conn, GDBusProxy *manager, const char *type)
 		return;
 	}
 
+	g_free(ad.type);
 	ad.type = g_strdup(type);
 
 	if (g_dbus_register_interface(conn, AD_PATH, AD_IFACE, ad_methods,
@@ -391,6 +392,9 @@ void ad_unregister(DBusConnection *conn, GDBusProxy *manager)
 	if (!ad.registered)
 		return;
 
+	g_free(ad.type);
+	ad.type = NULL;
+
 	if (g_dbus_proxy_method_call(manager, "UnregisterAdvertisement",
 					unregister_setup, unregister_reply,
 					conn, NULL) == FALSE) {
-- 
2.7.4

[PATCH BlueZ 2/6] client: Fix core dump when using set-advertise-name

[ERAMOTO Masaya]

After setting local name, if repeating to set on/off of set-advertise-name
then bluetoothctl may dump core due to double free. This patch uses
secure g_free() instead of free().
---
 client/advertising.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/client/advertising.c b/client/advertising.c
index b105da9..bfdebd5 100644
--- a/client/advertising.c
+++ b/client/advertising.c
@@ -547,8 +547,10 @@ void ad_advertise_name(DBusConnection *conn, bool value)
 
 	ad.name = value;
 
-	if (!value)
-		free(ad.local_name);
+	if (!value) {
+		g_free(ad.local_name);
+		ad.local_name = NULL;
+	}
 
 	g_dbus_emit_property_changed(conn, AD_PATH, AD_IFACE, "Includes");
 }
@@ -558,7 +560,7 @@ void ad_advertise_local_name(DBusConnection *conn, const char *name)
 	if (ad.local_name && !strcmp(name, ad.local_name))
 		return;
 
-	free(ad.local_name);
+	g_free(ad.local_name);
 	ad.local_name = strdup(name);
 
 	g_dbus_emit_property_changed(conn, AD_PATH, AD_IFACE, "LocalName");
-- 
2.7.4

[PATCH BlueZ 3/6] client: Prevent to pass invalid type to D-Bus

[ERAMOTO Masaya]

---
 client/advertising.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/client/advertising.c b/client/advertising.c
index bfdebd5..76cee3d 100644
--- a/client/advertising.c
+++ b/client/advertising.c
@@ -131,7 +131,7 @@ static gboolean get_type(const GDBusPropertyTable *property,
 {
 	const char *type = "peripheral";
 
-	if (!ad.type || strlen(ad.type) > 0)
+	if (ad.type && strlen(ad.type) > 0)
 		type = ad.type;
 
 	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &type);
-- 
2.7.4

[PATCH BlueZ 4/6] client: Use existing function for parsing argument

[ERAMOTO Masaya]

---
 client/main.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/client/main.c b/client/main.c
index 2cb449f..7b24633 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2368,22 +2368,12 @@ static void cmd_set_advertise_manufacturer(const char *arg)
 
 static void cmd_set_advertise_tx_power(const char *arg)
 {
-	if (arg == NULL || strlen(arg) == 0) {
-		rl_printf("Missing on/off argument\n");
-		return;
-	}
-
-	if (strcmp(arg, "on") == 0 || strcmp(arg, "yes") == 0) {
-		ad_advertise_tx_power(dbus_conn, true);
-		return;
-	}
+	dbus_bool_t powered;
 
-	if (strcmp(arg, "off") == 0 || strcmp(arg, "no") == 0) {
-		ad_advertise_tx_power(dbus_conn, false);
+	if (parse_argument_on_off(arg, &powered) == FALSE)
 		return;
-	}
 
-	rl_printf("Invalid argument\n");
+	ad_advertise_tx_power(dbus_conn, powered);
 }
 
 static void cmd_set_advertise_name(const char *arg)
-- 
2.7.4

[PATCH BlueZ 5/6] client: Introduce parse_argument()

[ERAMOTO Masaya]

---
 client/main.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/client/main.c b/client/main.c
index 7b24633..1c34269 100644
--- a/client/main.c
+++ b/client/main.c
@@ -845,6 +845,44 @@ static gboolean check_default_ctrl(void)
 	return TRUE;
 }
 
+static gboolean parse_argument(const char *arg, const char * const *arg_table,
+					const char *msg, dbus_bool_t *value,
+					const char **option)
+{
+	const char * const *opt;
+
+	if (!arg || !strlen(arg)) {
+		if (msg)
+			rl_printf("Missing on/off/%s argument\n", msg);
+		else
+			rl_printf("Missing on/off argument\n");
+		return FALSE;
+	}
+
+	if (!strcmp(arg, "on") || !strcmp(arg, "yes")) {
+		*value = TRUE;
+		if (option)
+			*option = "";
+		return TRUE;
+	}
+
+	if (!strcmp(arg, "off") || !strcmp(arg, "no")) {
+		*value = FALSE;
+		return TRUE;
+	}
+
+	for (opt = arg_table; opt && *opt; opt++) {
+		if (strcmp(arg, *opt) == 0) {
+			*value = TRUE;
+			*option = *opt;
+			return TRUE;
+		}
+	}
+
+	rl_printf("Invalid argument %s\n", arg);
+	return FALSE;
+}
+
 static gboolean parse_argument_on_off(const char *arg, dbus_bool_t *value)
 {
 	if (!arg || !strlen(arg)) {
-- 
2.7.4

[PATCH BlueZ 6/6] client: Use new parse_argument() function

[ERAMOTO Masaya]

---
 client/main.c | 105 ++++++----------------------------------------------------
 1 file changed, 10 insertions(+), 95 deletions(-)

diff --git a/client/main.c b/client/main.c
index 1c34269..b5646b2 100644
--- a/client/main.c
+++ b/client/main.c
@@ -883,60 +883,6 @@ static gboolean parse_argument(const char *arg, const char * const *arg_table,
 	return FALSE;
 }
 
-static gboolean parse_argument_on_off(const char *arg, dbus_bool_t *value)
-{
-	if (!arg || !strlen(arg)) {
-		rl_printf("Missing on/off argument\n");
-		return FALSE;
-	}
-
-	if (!strcmp(arg, "on") || !strcmp(arg, "yes")) {
-		*value = TRUE;
-		return TRUE;
-	}
-
-	if (!strcmp(arg, "off") || !strcmp(arg, "no")) {
-		*value = FALSE;
-		return TRUE;
-	}
-
-	rl_printf("Invalid argument %s\n", arg);
-	return FALSE;
-}
-
-static gboolean parse_argument_agent(const char *arg, dbus_bool_t *value,
-							const char **capability)
-{
-	const char * const *opt;
-
-	if (arg == NULL || strlen(arg) == 0) {
-		rl_printf("Missing on/off/capability argument\n");
-		return FALSE;
-	}
-
-	if (strcmp(arg, "on") == 0 || strcmp(arg, "yes") == 0) {
-		*value = TRUE;
-		*capability = "";
-		return TRUE;
-	}
-
-	if (strcmp(arg, "off") == 0 || strcmp(arg, "no") == 0) {
-		*value = FALSE;
-		return TRUE;
-	}
-
-	for (opt = agent_arguments; *opt; opt++) {
-		if (strcmp(arg, *opt) == 0) {
-			*value = TRUE;
-			*capability = *opt;
-			return TRUE;
-		}
-	}
-
-	rl_printf("Invalid argument %s\n", arg);
-	return FALSE;
-}
-
 static void cmd_list(const char *arg)
 {
 	GList *list;
@@ -1099,7 +1045,7 @@ static void cmd_power(const char *arg)
 	dbus_bool_t powered;
 	char *str;
 
-	if (parse_argument_on_off(arg, &powered) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &powered, NULL) == FALSE)
 		return;
 
 	if (check_default_ctrl() == FALSE)
@@ -1120,7 +1066,7 @@ static void cmd_pairable(const char *arg)
 	dbus_bool_t pairable;
 	char *str;
 
-	if (parse_argument_on_off(arg, &pairable) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &pairable, NULL) == FALSE)
 		return;
 
 	if (check_default_ctrl() == FALSE)
@@ -1141,7 +1087,7 @@ static void cmd_discoverable(const char *arg)
 	dbus_bool_t discoverable;
 	char *str;
 
-	if (parse_argument_on_off(arg, &discoverable) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &discoverable, NULL) == FALSE)
 		return;
 
 	if (check_default_ctrl() == FALSE)
@@ -1163,7 +1109,8 @@ static void cmd_agent(const char *arg)
 	dbus_bool_t enable;
 	const char *capability;
 
-	if (parse_argument_agent(arg, &enable, &capability) == FALSE)
+	if (parse_argument(arg, agent_arguments, "capability",
+					&enable, &capability) == FALSE)
 		return;
 
 	if (enable == TRUE) {
@@ -1213,7 +1160,7 @@ static void cmd_scan(const char *arg)
 	dbus_bool_t enable;
 	const char *method;
 
-	if (parse_argument_on_off(arg, &enable) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &enable, NULL) == FALSE)
 		return;
 
 	if (check_default_ctrl() == FALSE)
@@ -2041,7 +1988,7 @@ static void cmd_notify(const char *arg)
 {
 	dbus_bool_t enable;
 
-	if (parse_argument_on_off(arg, &enable) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &enable, NULL) == FALSE)
 		return;
 
 	if (!default_attr) {
@@ -2332,45 +2279,13 @@ static char *capability_generator(const char *text, int state)
 	return argument_generator(text, state, agent_arguments);
 }
 
-static gboolean parse_argument_advertise(const char *arg, dbus_bool_t *value,
-							const char **type)
-{
-	const char * const *opt;
-
-	if (arg == NULL || strlen(arg) == 0) {
-		rl_printf("Missing on/off/type argument\n");
-		return FALSE;
-	}
-
-	if (strcmp(arg, "on") == 0 || strcmp(arg, "yes") == 0) {
-		*value = TRUE;
-		*type = "";
-		return TRUE;
-	}
-
-	if (strcmp(arg, "off") == 0 || strcmp(arg, "no") == 0) {
-		*value = FALSE;
-		return TRUE;
-	}
-
-	for (opt = ad_arguments; *opt; opt++) {
-		if (strcmp(arg, *opt) == 0) {
-			*value = TRUE;
-			*type = *opt;
-			return TRUE;
-		}
-	}
-
-	rl_printf("Invalid argument %s\n", arg);
-	return FALSE;
-}
-
 static void cmd_advertise(const char *arg)
 {
 	dbus_bool_t enable;
 	const char *type;
 
-	if (parse_argument_advertise(arg, &enable, &type) == FALSE)
+	if (parse_argument(arg, ad_arguments, "type",
+					&enable, &type) == FALSE)
 		return;
 
 	if (!default_ctrl || !default_ctrl->ad_proxy) {
@@ -2408,7 +2323,7 @@ static void cmd_set_advertise_tx_power(const char *arg)
 {
 	dbus_bool_t powered;
 
-	if (parse_argument_on_off(arg, &powered) == FALSE)
+	if (parse_argument(arg, NULL, NULL, &powered, NULL) == FALSE)
 		return;
 
 	ad_advertise_tx_power(dbus_conn, powered);
-- 
2.7.4

Re: [PATCH BlueZ 5/6] client: Introduce parse_argument()

[Luiz Augusto von Dentz]

Hi Eramoto,

On Fri, Sep 22, 2017 at 3:20 PM, ERAMOTO Masaya
<eramoto.masaya@jp.fujitsu.com> wrote:
> ---
>  client/main.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index 7b24633..1c34269 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -845,6 +845,44 @@ static gboolean check_default_ctrl(void)
>         return TRUE;
>  }
>
> +static gboolean parse_argument(const char *arg, const char * const *arg_=
table,
> +                                       const char *msg, dbus_bool_t *val=
ue,
> +                                       const char **option)
> +{
> +       const char * const *opt;
> +
> +       if (!arg || !strlen(arg)) {
> +               if (msg)
> +                       rl_printf("Missing on/off/%s argument\n", msg);
> +               else
> +                       rl_printf("Missing on/off argument\n");
> +               return FALSE;
> +       }
> +
> +       if (!strcmp(arg, "on") || !strcmp(arg, "yes")) {
> +               *value =3D TRUE;
> +               if (option)
> +                       *option =3D "";
> +               return TRUE;
> +       }
> +
> +       if (!strcmp(arg, "off") || !strcmp(arg, "no")) {
> +               *value =3D FALSE;
> +               return TRUE;
> +       }
> +
> +       for (opt =3D arg_table; opt && *opt; opt++) {
> +               if (strcmp(arg, *opt) =3D=3D 0) {
> +                       *value =3D TRUE;
> +                       *option =3D *opt;
> +                       return TRUE;
> +               }
> +       }
> +
> +       rl_printf("Invalid argument %s\n", arg);
> +       return FALSE;
> +}
> +
>  static gboolean parse_argument_on_off(const char *arg, dbus_bool_t *valu=
e)
>  {
>         if (!arg || !strlen(arg)) {
> --
> 2.7.4

This one if applied separately would break bisect:

client/main.c:848:17: error: =E2=80=98parse_argument=E2=80=99 defined but n=
ot used
[-Werror=3Dunused-function]
 static gboolean parse_argument(const char *arg, const char * const *arg_ta=
ble,
                 ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Lets have it merged with 6/6.


>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--=20
Luiz Augusto von Dentz