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
Hi Eramoto,
On Fri, Sep 22, 2017 at 3:20 PM, ERAMOTO Masaya
<[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--=20
Luiz Augusto von Dentz
---
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
---
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
---
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
---
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
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
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