This patch set fixes bugs of advertising.c and refactors
advertise-related code.
Changes since v1:
- Merge 5th and 6th patches into a new patch
ERAMOTO Masaya (5):
client: Fix memory leak of advertise command
client: Fix core dump when using set-advertise-name
client: Prevent to pass invalid ad type to D-Bus
client: Use existing function for parsing argument
client: Use new parse_argument() instead of parse_argument_XX()
client/advertising.c | 14 +++++--
client/main.c | 107 ++++++++++++---------------------------------------
2 files changed, 35 insertions(+), 86 deletions(-)
--
2.7.4
Hi Eramoto,
On Mon, Sep 25, 2017 at 7:21 AM, ERAMOTO Masaya
<[email protected]> wrote:
> This patch set fixes bugs of advertising.c and refactors
> advertise-related code.
>
> Changes since v1:
> - Merge 5th and 6th patches into a new patch
>
> ERAMOTO Masaya (5):
> client: Fix memory leak of advertise command
> client: Fix core dump when using set-advertise-name
> client: Prevent to pass invalid ad type to D-Bus
> client: Use existing function for parsing argument
> client: Use new parse_argument() instead of parse_argument_XX()
>
> client/advertising.c | 14 +++++--
> client/main.c | 107 ++++++++++++---------------------------------------
> 2 files changed, 35 insertions(+), 86 deletions(-)
>
> --
> 2.7.4
Applied, thanks.
Note that Ive changed it a little bit since I did push a patch
removing the const char * const construct. Also this consolidation of
parsing might actually be better put into an argument struct to be
part of the command table e.g.:
struct bt_shell_arg {
const char **options;
bool required;
};
The options array can be used for both parsing and auto complete that
way we can have a function that parses them before the actual callback
of cmd_table, though I think this should go directly to bt_shell so
every tool would have the same logic.
--
Luiz Augusto von Dentz
---
client/main.c | 93 +++++++++++++++--------------------------------------------
1 file changed, 23 insertions(+), 70 deletions(-)
diff --git a/client/main.c b/client/main.c
index 3dadd5b..2584408 100644
--- a/client/main.c
+++ b/client/main.c
@@ -845,15 +845,24 @@ static gboolean check_default_ctrl(void)
return TRUE;
}
-static gboolean parse_argument_on_off(const char *arg, dbus_bool_t *value)
+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)) {
- rl_printf("Missing on/off argument\n");
+ 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;
}
@@ -862,35 +871,10 @@ static gboolean parse_argument_on_off(const char *arg, dbus_bool_t *value)
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++) {
+ for (opt = arg_table; opt && *opt; opt++) {
if (strcmp(arg, *opt) == 0) {
*value = TRUE;
- *capability = *opt;
+ *option = *opt;
return TRUE;
}
}
@@ -1061,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)
@@ -1082,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)
@@ -1103,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)
@@ -1125,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) {
@@ -1175,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)
@@ -2003,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) {
@@ -2294,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) {
@@ -2370,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
---
client/main.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/client/main.c b/client/main.c
index 91b728a..3dadd5b 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
---
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
If repeating to set on/off with set-advertise-name after setting local
name, and then may dump core by double free. This patch uses 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
Since advertise command does not free the variable ad.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