[RFC PATCH v3 1/1] atmodem/lte: proto and authentication handling

[Jonas Bonn]

Hi,

This looks reasonable from my point of view.  Two little comments below...

/Jonas

The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.
---
drivers/atmodem/lte.c | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e5af6cd2 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
* oFono - Open Source Telephony
*
* Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Copyright (C) 2018 Gemalto M2M
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -45,10 +46,17 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_callbackdata {
+ ofono_lte_cb_t cb;
+ void *data;
+ struct lte_driver_data *ldd;
+ const struct ofono_lte_default_attach_info *info;
+};
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct lte_callbackdata *cbd = user_data;
ofono_lte_cb_t cb = cbd->cb;
struct ofono_error error;
@@ -58,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct lte_callbackdata *cbd = user_data;
+ ofono_lte_cb_t cb = cbd->cb;
+ void *data = cbd->data;
+ struct ofono_error error;
+ char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+ size_t buflen = sizeof(buf);
+ size_t len;
+ enum ofono_gprs_auth_method auth_method;
+
+ if (!ok) {
+ g_free(cbd);
+ decode_at_error(&error, g_at_result_final_response(result));
+ cb(&error, data);

You'll want a return here, too.

+ }
+
+ len = snprintf(buf, buflen, "AT+CGAUTH=0,");
+ buflen -= len;
+ auth_method = cbd->info->auth_method;
+
+ /* change the authentication method if the parameters are invalid */
+ if (!*cbd->info->username || !*cbd->info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch (auth_method) {
+ snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ snprintf(buf + len, buflen, "0");
+ break;
+ }
+
+ if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, g_free) > 0)
+ return;
+

Here you'll leak cbd again.

+ CALLBACK_WITH_FAILURE(cb, data);
+}
+
static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info,
ofono_lte_cb_t cb, void *data)
{
struct lte_driver_data *ldd = ofono_lte_get_data(lte);
char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
- struct cb_data *cbd = cb_data_new(cb, data);
+ struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
+ const char *proto;
+ size_t len;
DBG("LTE config with APN: %s", info->apn);
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;
+ cbd->info = info;
+
+ switch (info->proto) {
+ proto = "IPV6";
+ break;
+ proto = "IPV4V6";
+ break;
+ proto = "IP";
+ break;
+ }
+
+ len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+ if (*info->apn)
+ snprintf(buf+len, sizeof(buf)-len, ",\"%s\"", info->apn);
- /* We can't do much in case of failure so don't check response. */
if (g_at_chat_send(ldd->chat, buf, NULL,
- at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+ at_lte_set_default_attach_info_cb, cbd, NULL) > 0)
return;
+ g_free(cbd);
CALLBACK_WITH_FAILURE(cb, data);
}

[Jonas Bonn]

Post by Jonas Bonn

+
+ if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, g_free) > 0)
+ return;
+

Here you'll leak cbd again.

here there is a g_free in the call. Also in case of failure the g_free
is executed, I think (also because it is the same construct as
everywhere else).
Or not?

If g_at_chat_send returns 0, g_free() won't be called so you need to
free the data yourself.  That should be the construct used elsewhere so
if you see it done differently your seeing bugs (probably).

/Jonas

[Denis Kenzior]

Hi Giacinto,

Hi,
I trust you, but I have checked the code :)
you are right, the at_command_create may fail (on allocation error
from g_try_new0) and in this case it doesn't call the g_free.
However I have to say that the code leaks alot: overall the construct
is used 237 times, and only 98 times the g_free is called.

Umm, can you give me some examples? Do note that we have 2 patterns
that we use:

Pattern 1 is the most common:

if (g_at_chat_send(...) > 0)
return;

g_free/destroy(...);
CALLBACK_WITH_FAILURE();

Pattern 2 is what you want:

if (g_at_chat_send(...) == 0) {
g_free/destroy(...);
CALLBACK_WITH_FAILURE();
return;
}

That said, according to Denis nowadays we don't use g_try_new0 because
the allocation won't fail, so we are unlikely to see it in action.

That is not really an excuse, and we can change the implementation of
send() at any time. So you still need to be paranoid.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

I have already added it.
I have just greped through the code, here are some examples, from the
plugins/phonesim.c:18187: if (g_at_chat_send(data->chat, buf, none_prefix,
plugins/phonesim.c-18237- set_online_cb, cbd, g_free) > 0)
plugins/phonesim.c-18274- return;
plugins/phonesim.c-18284-
plugins/phonesim.c-18285- CALLBACK_WITH_FAILURE(cb, user_data);

Okay, that's a bug. Fixed.

gatchat/gatmux.c:19544: if (g_at_chat_send(chat, "AT+CMUX=?", cmux_prefix,
gatchat/gatmux.c-19596- mux_query_cb, msd, msd_free) > 0)
gatchat/gatmux.c-19634- return TRUE;
gatchat/gatmux.c-19649-
gatchat/gatmux.c-19650- if (msd)
gatchat/gatmux.c-19660- msd_free(msd);
// this is actually a mistake because the msd pointer won't change, freed or not

I don't see anything wrong here? Besides the redundant if which will
always be true.

drivers/xmm7modem/netmon.c:5135: if (g_at_chat_send(nmd->chat,
"AT+XMCI=1", xmci_prefix,
drivers/xmm7modem/netmon.c-5192- xmci_cb, cbd, g_free) > 0)
drivers/xmm7modem/netmon.c-5223- return;
drivers/xmm7modem/netmon.c-5233-
drivers/xmm7modem/netmon.c-5234- CALLBACK_WITH_FAILURE(cb, data);

And fixed this as well.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.
---
drivers/atmodem/lte.c | 94 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 10 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..e5af6cd2 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
* oFono - Open Source Telephony
*
* Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Copyright (C) 2018 Gemalto M2M
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -45,10 +46,17 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_callbackdata {
+ ofono_lte_cb_t cb;
+ void *data;
+ struct lte_driver_data *ldd;
+ const struct ofono_lte_default_attach_info *info;
+};
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct lte_callbackdata *cbd = user_data;
ofono_lte_cb_t cb = cbd->cb;
struct ofono_error error;
@@ -58,27 +66,93 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct lte_callbackdata *cbd = user_data;
+ ofono_lte_cb_t cb = cbd->cb;
+ void *data = cbd->data;
+ struct ofono_error error;
+ char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+ size_t buflen = sizeof(buf);
+ size_t len;
+ enum ofono_gprs_auth_method auth_method;
+
+ if (!ok) {
+ g_free(cbd);
+ decode_at_error(&error, g_at_result_final_response(result));
+ cb(&error, data);
+ }
+
+ len = snprintf(buf, buflen, "AT+CGAUTH=0,");
+ buflen -= len;
+ auth_method = cbd->info->auth_method;
+
+ /* change the authentication method if the parameters are invalid */
+ if (!*cbd->info->username || !*cbd->info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch (auth_method) {
+ snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+ break;
+ snprintf(buf + len, buflen, "0");
+ break;
+ }
+
+ if (g_at_chat_send(cbd->ldd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, g_free) > 0)
+ return;
+
+ CALLBACK_WITH_FAILURE(cb, data);
+}
+
static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info,
ofono_lte_cb_t cb, void *data)
{
struct lte_driver_data *ldd = ofono_lte_get_data(lte);
char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
- struct cb_data *cbd = cb_data_new(cb, data);
+ struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
+ const char *proto;
+ size_t len;
DBG("LTE config with APN: %s", info->apn);
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;

You can't really do that. There's no guarantee that the core atom will
keep this object around for the lifetime of the driver transaction. Why
do you need this anyway? Is it just to build the +CGAUTH part? Why
don't you l_strdup_printf the +CGAUTH commmand or something instead and
invoke it in the callback?

E.g. cbd = cb_data_new();
char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s", username,
password);

cbd->user = cgauth_cmd;

Alternatively you can queue both commands and save the +CGAUTH command
id inside ldd. Then if +CGDCONT fails, you can g_at_chat_cancel the
+CGAUTH.

+ cbd->info = info;
+
+ switch (info->proto) {
+ proto = "IPV6";
+ break;
+ proto = "IPV4V6";
+ break;
+ proto = "IP";
+ break;
+ }
+
+ len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+ if (*info->apn)
+ snprintf(buf+len, sizeof(buf)-len, ",\"%s\"", info->apn);
- /* We can't do much in case of failure so don't check response. */
if (g_at_chat_send(ldd->chat, buf, NULL,
- at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+ at_lte_set_default_attach_info_cb, cbd, NULL) > 0)

Why are you removing the destructor. This will cause leaks whenever a
hot-unplug event happens and this command is queued...

If you want to use cbd across multiple commands, the proper way to do
that is with a reference counted structure.

return;
+ g_free(cbd);
CALLBACK_WITH_FAILURE(cb, data);
}

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior

+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->ldd = ldd;

You can't really do that. There's no guarantee that the core atom will
keep this object around for the lifetime of the driver transaction.

That's interesting. Can I pass const struct ofono_lte *lte, or the
same constraints apply?
Or some other object that I can rely upon?
Also, the core atom shouldn't be called until the callback is triggered.

Why would you need to? In general one can assume that the atom is
around until the .remove method is called. But you cannot assume that
anything passed into the driver will live past the point that the driver
function returns.

Post by Denis Kenzior
Why do you need this anyway? Is it just to build the +CGAUTH part? Why
don't you l_strdup_printf the +CGAUTH commmand or something instead and
invoke it in the callback?
E.g. cbd = cb_data_new();
char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s", username,
password);

I will check this, also for compatibility with the switch(vendor) to
come, but I have to confess
I don't really like building the command somewhere and using it later.
It is unexpected for someone reading the code.
What if I have to chain more commands?

Then memcpy the pending structure (or the needed members) into an area
with lifetime you control.

Post by Denis Kenzior
Why are you removing the destructor. This will cause leaks whenever a
hot-unplug event happens and this command is queued...
If you want to use cbd across multiple commands, the proper way to do
that is with a reference counted structure.

is there an example elsewhere in the code?

drivers/ubloxmodem/netmon.c
drivers/hfpmodem/slc.c

There may be a few other places that get this wrong and should be fixed.
Feel free to point them out when/if you see these.

Regards,
-Denis