From: Grant Erickson <gerickson@nuovations.com>
To: connman@lists.linux.dev
Subject: Re: Update on Chasing Down a Crash in connman_service_get_nameservers on Cellular Service Removal
Date: Wed, 1 Nov 2023 14:30:45 -0700 [thread overview]
Message-ID: <32329477-767D-4FAC-8222-A309F1CA1114@nuovations.com> (raw)
In-Reply-To: <FE5E3686-1DFC-4216-9A57-90A6114F279D@nuovations.com>
On Oct 31, 2023, at 5:44 PM, Grant Erickson <gerickson@nuovations.com> wrote:
> On Oct 31, 2023, at 12:18 PM, Grant Erickson <gerickson@nuovations.com> wrote:
>> On Oct 19, 2023, at 5:48 PM, Grant Erickson <gerickson@nuovations.com> wrote:
>>> On Oct 18, 2023, at 8:00 PM, Grant Erickson <gerickson@nuovations.com> wrote:
>>>> I know some of you (Denis, I believe from git log history) are working on clang static and dynamic analysis for connman.
>>>>
>>>> To the extent it helps, I have been chasing down an issue in top-of-tree (53964711baee916ae163e64d5229e3822935e100) in which when Quectel / Nimblelink QMI cellular goes away on my system, I get a connman signal 11 abort. Unfortunately, there is no backtrace in this case since n_ptrs (0) is less than offset (2) in the print_backtrace call.
>>>>
>>>> Today, doing some other development and debug work, I was able to narrow down the downstream cause of the abort. It seems that things are crashing somewhere within src/service.c:connman_service_get_nameservers().
>>>>
>>>> When name servers are established for the cellular service from the provider, there end up being two (2) name servers. The core problem seems to be that g_strv_length(service->nameservers) incorrectly returns 7 (it really only has two (2)) which results in:
>>>>
>>>> for (i = 0; i < len; i++)
>>>> nameservers[i] = g_strdup(service->nameservers[i]);
>>>>
>>>> attempting to copy strings that do not exist past the end of the actual bounds of service->nameservers. So, it would seem that at some point, service->nameservers becomes corrupt with its terminating null getting overwritten by something else. I’ve get to zero in on the nature of “getting overwritten by something else”.
>>>>
>>>> I also periodically get this in the output after an abort:
>>>>
>>>> malloc(): smallbin double linked list corrupted
>>>>
>>>> I am also able to cause the abort, nearly on demand with near 100% reproducibility, by issuing ‘connmanctl disable cellular’.
ConnMan Community:
I am able to confirm that the following patch addresses the heap-use-after-free issue when Cellular (or, in fact, any network service goes away):
diff --git a/src/service.c b/src/service.c
--- a/src/service.c
+++ b/src/service.c
@@ -6527,11 +6651,12 @@
int __connman_service_ipconfig_indicate_state(struct connman_service *service,
else
service->state_ipv6 = new_state;
- if (!is_connected(old_state) && is_connected(new_state))
+ if (!is_connected(old_state) && is_connected(new_state)) {
nameserver_add_all(service, type);
- __connman_timeserver_sync(service,
- CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE);
+ __connman_timeserver_sync(service,
+ CONNMAN_TIMESERVER_SYNC_REASON_STATE_UPDATE);
+ }
return service_indicate_state(service);
}
There could also be an argument made that 'ts_service' should be a strong (rather than a weak) reference using ‘connman_service_ref' and ‘connman_service_unref’, as follows:
diff --git a/third_party/connman/repo/src/timeserver.c b/third_party/connman/repo/src/timeserver.c
index d23776faa96c..e91715b6452d 100644
--- a/third_party/connman/repo/src/timeserver.c
+++ b/third_party/connman/repo/src/timeserver.c
@@ -427,6 +427,7 @@
static void ts_reset(struct connman_service *service)
ts_recheck_enable();
+ connman_service_ref(service);
ts_service = service;
timeserver_sync_start();
}
@@ -506,7 +507,10 @@
static void timeserver_stop(void)
{
DBG(" ");
- ts_service = NULL;
+ if (ts_service) {
+ connman_service_unref(ts_service);
+ ts_service = NULL;
+ }
if (resolv) {
g_resolv_unref(resolv);
Thoughts?
I’ll follow up with a formal patch submission for the first patch shortly.
Best,
Grant
--
Principal
Nuovations
gerickson@nuovations.com
http://www.nuovations.com/
prev parent reply other threads:[~2023-11-01 21:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-19 3:00 Update on Chasing Down a Crash in connman_service_get_nameservers on Cellular Service Removal Grant Erickson
2023-10-20 0:48 ` Grant Erickson
2023-10-31 19:18 ` Grant Erickson
2023-11-01 0:44 ` Grant Erickson
2023-11-01 21:30 ` Grant Erickson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=32329477-767D-4FAC-8222-A309F1CA1114@nuovations.com \
--to=gerickson@nuovations.com \
--cc=connman@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).