Re: [PATCH] dnsproxy: request_data in request_list need the request data
Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: The ones received over UDP didn't have it. --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } + req-name = g_strdup(query); + req-request = g_malloc(len); + memcpy(req-request, buf, len); req-timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? Jukka, any comments? Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: remove_from_network should drop reference to the network
Hi,
On Sun, 2015-04-05 at 14:22 +0300, Slava Monich wrote:
Otherwise the service may remain associated with a dead network.
In addition to that, update_from_network() now checks that session
is associated with the correct network. Without this check, association
with a dead network in the connecting state was unrecoverable.
---
src/service.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/src/service.c b/src/service.c
index 7538bdd..6d3414d 100644
--- a/src/service.c
+++ b/src/service.c
@@ -6616,6 +6616,11 @@ static void update_from_network(struct connman_service
*service,
if (is_connected(service))
return;
+ if (service-network service-network != network) {
+ connman_network_unref(service-network);
+ service-network = NULL;
+ }
+
if (is_connecting(service))
return;
A service corresponding to a network is looked up in
__connman_service_create_from_network() using the network ident (not the
pointer itself). Thus the service and the network belong to each other.
The network plugins (must) ensure that network idents are unique.
It seems Jolla's other patches are a source of confusion here, last time
I looked services were created without valid network structures to back
them up. Please at some point fix such things and move UI layer things
to the UI, not ConnMan.
@@ -6863,6 +6868,11 @@ void __connman_service_remove_from_network(struct
connman_network *network)
__connman_connection_gateway_remove(service,
CONNMAN_IPCONFIG_TYPE_ALL);
+ if (service-network) {
+ connman_network_unref(service-network);
+ service-network = NULL;
+ }
+
connman_service_unref(service);
}
No network, no service. The network unref is done with
g_hash_table_remove() in connman_service_unref().
Again Jolla's additional patches break this assumption as you may have
one more reference for the service not to loose a network-less one from
the list, right?
Cheers,
Patrik
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
On Sun, 2015-04-05 at 14:35 +0300, Slava Monich wrote: I think I finally caught one of the scenarios like that. The trick was to receive DISCONNECTED event from wpa_supplicant followed by ASSOCIATING while network-connecting is set to true. 1. DISCONNECTED sets device-scanning to true 2. ASSOCIATING sets device-scanning to false, which invokes remove_unavailable_network(), sets network-driver to NULL and eventually calls __connman_service_remove_from_network() for the network in question. Now the service is holding reference to the dead network, update_from_network() doesn't work because network-connecting is true, __connman_network_disconnect() doesn't work because network-driver is NULL. That's it. I just sent a patch that should prevent that from happening in two places along this course of events - by dropping reference to the dead service in __connman_service_remove_from_network() and in update_from_network(). Under this particular scenario the latter seems to be unnecessary but I'm pretty sure there are more bugs floating around so I suggest to keep both checks. This is way too late/too high in the stack. Please fix this latest in plugins/wifi.c if gsupplicant cannot handle this. Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: request_data in request_list need the request data
On 07/04/15 12:14, Patrik Flykt wrote: Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: The ones received over UDP didn't have it. --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } +req-name = g_strdup(query); +req-request = g_malloc(len); +memcpy(req-request, buf, len); req-timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? |==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: remove_from_network should drop reference to the network
On 07/04/15 12:42, Patrik Flykt wrote:
@@ -6863,6 +6868,11 @@ void __connman_service_remove_from_network(struct
connman_network *network)
__connman_connection_gateway_remove(service,
CONNMAN_IPCONFIG_TYPE_ALL);
+if (service-network) {
+connman_network_unref(service-network);
+service-network = NULL;
+}
+
connman_service_unref(service);
}
No network, no service. The network unref is done with
g_hash_table_remove() in connman_service_unref().
Again Jolla's additional patches break this assumption as you may have
one more reference for the service not to loose a network-less one from
the list, right?
The fewer assumptions the better.
The code which calls unref() can't make the assumption that it's the
last reference and the object gets deallocated. Otherwise why would you
use refcount in the first place if you know exactly when to deallocate
the object, right?
I'm reasonably sure that in Jolla code the additional reference is held
by the wispr context. Otherwise it would crash there.
-Slava
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: request_data in request_list need the request data
On ti, 2015-04-07 at 12:58 +0300, Slava Monich wrote: On 07/04/15 12:14, Patrik Flykt wrote: Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: The ones received over UDP didn't have it. --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } + req-name = g_strdup(query); + req-request = g_malloc(len); + memcpy(req-request, buf, len); req-timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? |==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| FYI, something like this trace would be really nice to see in the commit message. Cheers, Jukka ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: request_data in request_list need the request data
On 07/04/15 13:40, Jukka Rissanen wrote: On ti, 2015-04-07 at 12:58 +0300, Slava Monich wrote: On 07/04/15 12:14, Patrik Flykt wrote: Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: The ones received over UDP didn't have it. --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } + req-name = g_strdup(query); + req-request = g_malloc(len); + memcpy(req-request, buf, len); req-timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? |==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| FYI, something like this trace would be really nice to see in the commit message. OK, will do next time around. I have valgrind/gdb backtraces for most patches I send to the list, I just wasn't sure if it would be appropriate here. Here is one for the agent.c patch I sent a few days ago: https://github.com/mer-packages/connman/pull/210 Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] ipv6pd: Check timer_hash for NULL prior to destroying it
GLib doesn't like NULL pointers.
cleanup() may be invoked more than once after successful setup,
first from dhcpv6_callback() and then from __connman_ipv6pd_cleanup()
---
src/ipv6pd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/ipv6pd.c b/src/ipv6pd.c
index 5ecda38..0d221f0 100644
--- a/src/ipv6pd.c
+++ b/src/ipv6pd.c
@@ -165,8 +165,10 @@ static void cleanup(void)
timer_uplink = 0;
}
- g_hash_table_destroy(timer_hash);
- timer_hash = NULL;
+ if (timer_hash) {
+ g_hash_table_destroy(timer_hash);
+ timer_hash = NULL;
+ }
if (prefixes) {
g_slist_free_full(prefixes, g_free);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[RFC 2/3] dnsproxy: Do not check numserv in request timeout
The req-numserv tries to track that we have received all the
answers from different servers. This is pointless when the request
is about to go away as we need to send something to the client
in this case anyway.
---
src/dnsproxy.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 0698387..2a0a94a 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -538,7 +538,6 @@ static gboolean request_timeout(gpointer user_data)
DBG(id 0x%04x, req-srcid);
request_list = g_slist_remove(request_list, req);
- req-numserv--;
if (req-resplen 0 req-resp) {
int sk, err;
@@ -558,7 +557,7 @@ static gboolean request_timeout(gpointer user_data)
}
if (err 0)
return FALSE;
- } else if (req-request req-numserv == 0) {
+ } else if (req-request) {
struct domain_hdr *hdr;
if (req-protocol == IPPROTO_TCP) {
--
2.1.0
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[RFC 0/3] DNS proxy fixes
Hi, Slava noticed that dnsproxy.c does not set request buffer for UDP requests but leaves it NULL. This is fixed in patch 1 by Slava. I added that patch into this set (and added more details into commit message) as this is very much RFC material. After this patchset the UDP packet resending when the resolver is flushed starts to work after many years of failure. The resolver is flushed every time name servers are updated. This happens even if we disconnect a service which is very much pointless (some more patches will be sent to fix this later). What all this means, is that the UDP packet re-send and failure notifications to calling resolver client has not been working at all for some time. For UDP packets that the dnsproxy sends to external DNS servers, the req-request buffer was NULL. This then meant that the dnsproxy code that tries to send message to client in a case of failure cannot really do anything. The successful request was passed correctly to the client that sent the initial DNS request. So the issue has been the failure case when the external DNS server is not responding. There should be no similar issues in TCP side. Fortunately no one has reported this issue as the client app resolver (libc) still has its own timeout and everything is fine if there is an resolver error in ConnMan side. Anyway, the changes in patch 1 activate now some code paths that have not been in use for some time. So this set needs more testing in order not to introduce more bugs. Because everything seems to work just fine at the moment with the current code, it is quite possible that we could remove even more extra cruft from dnsproxy.c instead of introducing these fixes in this set. So if we do not apply patch 1, we could save some memory by removing dead code. By applying patch 1, more memory is consumed as we need to save the request buffer for each UDP DNS request. Patch 2 removes the numserv check in request_timeout() because it is pointless as we need to send some data to client anyway. Earlier this code path was not taken for UDP packets (because the req-request buffer was NULL). I also refactored the request_timeout() in patch 3 as it had duplicate code and was hard to follow. Cheers, Jukka Jukka Rissanen (2): dnsproxy: Do not check numserv in request timeout dnsproxy: Refactor the request_timeout() function Slava Monich (1): dnsproxy: request_data in request_list need the request data for UDP src/dnsproxy.c | 67 -- 1 file changed, 32 insertions(+), 35 deletions(-) -- 2.1.0 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[RFC 1/3] dnsproxy: request_data in request_list need the request data for UDP
From: Slava Monich slava.mon...@jolla.com UDP packet did not set the request and request_len properly. This is seen in valgrind report. ==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } + req-name = g_strdup(query); + req-request = g_malloc(len); + memcpy(req-request, buf, len); req-timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); -- 2.1.0 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
