Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH] gdhcp: use opened listening socket to send DHCP
renew request (Patrik Flykt)
2. Re: Delete WiFi access points via dbus (Patrik Flykt)
3. Re: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Patrik Flykt)
4. Re: Fwd: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Patrik Flykt)
5. Re: [PATCH] gdhcp: Don't send DHCPREQUEST if last assigned IP
is Link Local Address (Patrik Flykt)
6. Re: [PATCH] dhcpv6: Fix memory leak if for loop breaks
(Patrik Flykt)
7. Re: [PATCH] core: No need to compare expression against NULL
(Patrik Flykt)
8. [RFC v2] connection: Move online service to ready if gateway
is removed (Patrik Flykt)
----------------------------------------------------------------------
Message: 1
Date: Fri, 12 Feb 2016 10:39:30 +0200
From: Patrik Flykt <[email protected]>
To: Feng Wang <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] gdhcp: use opened listening socket to send DHCP
renew request
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Fri, 2016-01-29 at 16:47 -0800, Feng Wang wrote:
> It fix DHCP ACK lost issue when doing DHCP renewal.
> When doing DHCP renew, 2 sockets are opened. One is for
> listening DHCP ACK, the other is for transmitting DHCP request
> which is closed immediately after transmitting is done. But in
> some cases, the socket is closed after the DHCP ACK is received.
Are there any more details on the "some cases" ?
> The kernel will route the packet to the transmitting socket
> because it has a better match result(dst ip/port etc). And the
> packet was dropped when the socket was closed.
> ---
> gdhcp/client.c | 6 ++++--
> gdhcp/common.c | 60
> +++++++++++++++++++++++++++++++++++-----------------------
> gdhcp/common.h | 2 +-
> 3 files changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/gdhcp/client.c b/gdhcp/client.c
> index 3bf8cb2..ad587b1 100644
> --- a/gdhcp/client.c
> +++ b/gdhcp/client.c
> @@ -502,7 +502,8 @@ static int send_request(GDHCPClient *dhcp_client)
> if (dhcp_client->state == RENEWING)
> return dhcp_send_kernel_packet(&packet,
> dhcp_client->requested_ip, CLIENT_PORT,
> - dhcp_client->server_ip, SERVER_PORT);
> + dhcp_client->server_ip, SERVER_PORT,
> + dhcp_client->listener_sockfd);
>
> return dhcp_send_raw_packet(&packet, INADDR_ANY, CLIENT_PORT,
> INADDR_BROADCAST, SERVER_PORT,
> @@ -526,7 +527,8 @@ static int send_release(GDHCPClient *dhcp_client,
> dhcp_add_option_uint32(&packet, DHCP_SERVER_ID, server);
>
> return dhcp_send_kernel_packet(&packet, ciaddr, CLIENT_PORT,
> - server, SERVER_PORT);
> + server, SERVER_PORT,
> + dhcp_client->listener_sockfd);
> }
>
> static gboolean ipv4ll_probe_timeout(gpointer dhcp_data);
> diff --git a/gdhcp/common.c b/gdhcp/common.c
> index f3d4677..f0a9aa6 100644
> --- a/gdhcp/common.c
> +++ b/gdhcp/common.c
> @@ -626,44 +626,56 @@ int dhcp_send_raw_packet(struct dhcp_packet *dhcp_pkt,
>
> int dhcp_send_kernel_packet(struct dhcp_packet *dhcp_pkt,
> uint32_t source_ip, int source_port,
> - uint32_t dest_ip, int dest_port)
> + uint32_t dest_ip, int dest_port, int fd)
> {
> struct sockaddr_in client;
> - int fd, n, opt = 1;
> + int n, opt = 1;
>
> enum {
> DHCP_SIZE = sizeof(struct dhcp_packet) -
> EXTEND_FOR_BUGGY_SERVERS,
> };
>
> - fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_UDP);
> - if (fd < 0)
> - return -errno;
> + if (fd < 0) {
> + /* no socket opened, open a new socket to tx the packet and
> close it */
> + fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_UDP);
> + if (fd < 0)
> + return -errno;
> +
> + setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
> +
> + memset(&client, 0, sizeof(client));
> + client.sin_family = AF_INET;
> + client.sin_port = htons(source_port);
> + client.sin_addr.s_addr = htonl(source_ip);
> + if (bind(fd, (struct sockaddr *) &client, sizeof(client)) < 0) {
> + close(fd);
> + return -errno;
Here -errno is returned for the close() function, which succeeds. So the
returned error is zero. Please fix.
> + }
>
> - setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
> + memset(&client, 0, sizeof(client));
> + client.sin_family = AF_INET;
> + client.sin_port = htons(dest_port);
> + client.sin_addr.s_addr = htonl(dest_ip);
> + if (connect(fd, (struct sockaddr *) &client, sizeof(client)) <
> 0) {
> + close(fd);
> + return -errno;
> + }
This part is repeated below, please combine.
> - memset(&client, 0, sizeof(client));
> - client.sin_family = AF_INET;
> - client.sin_port = htons(source_port);
> - client.sin_addr.s_addr = htonl(source_ip);
> - if (bind(fd, (struct sockaddr *) &client, sizeof(client)) < 0) {
> - close(fd);
> - return -errno;
> - }
> + n = write(fd, dhcp_pkt, DHCP_SIZE);
>
> - memset(&client, 0, sizeof(client));
> - client.sin_family = AF_INET;
> - client.sin_port = htons(dest_port);
> - client.sin_addr.s_addr = htonl(dest_ip);
> - if (connect(fd, (struct sockaddr *) &client, sizeof(client)) < 0) {
> close(fd);
Can the socket really be closed here so that we don't repeat the
original problem?
> - return -errno;
> + } else {
> + /* Using existed socket to transmit the packet */
> + memset(&client, 0, sizeof(client));
> + client.sin_family = AF_INET;
> + client.sin_port = htons(dest_port);
> + client.sin_addr.s_addr = htonl(dest_ip);
> +
> + n = sendto(fd, dhcp_pkt, DHCP_SIZE, MSG_DONTWAIT,
> + (struct sockaddr *) &client, sizeof(client));
> }
>
> - n = write(fd, dhcp_pkt, DHCP_SIZE);
> -
> - close(fd);
> -
> if (n < 0)
> return -errno;
>
> diff --git a/gdhcp/common.h b/gdhcp/common.h
> index 75abc18..b92d214 100644
> --- a/gdhcp/common.h
> +++ b/gdhcp/common.h
> @@ -209,7 +209,7 @@ int dhcp_send_raw_packet(struct dhcp_packet *dhcp_pkt,
> int dhcpv6_send_packet(int index, struct dhcpv6_packet *dhcp_pkt, int len);
> int dhcp_send_kernel_packet(struct dhcp_packet *dhcp_pkt,
> uint32_t source_ip, int source_port,
> - uint32_t dest_ip, int dest_port);
> + uint32_t dest_ip, int dest_port, int fd);
> int dhcp_l3_socket(int port, const char *interface, int family);
> int dhcp_recv_l3_packet(struct dhcp_packet *packet, int fd);
> int dhcpv6_recv_l3_packet(struct dhcpv6_packet **packet, unsigned char *buf,
Patrik
------------------------------
Message: 2
Date: Fri, 12 Feb 2016 10:43:01 +0200
From: Patrik Flykt <[email protected]>
To: Pushkin Andrei <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: Delete WiFi access points via dbus
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Wed, 2016-02-10 at 11:51 +0000, Pushkin Andrei wrote:
> Hi. How I can delete all that connman know about AP (like rm
> -rf /var/lib/connman/wifi_SOME_THING) via dbus?
See doc/service-api.txt, connmanctl config <serviceXXXX> remove or
test/test-connman remove.
Patrik
------------------------------
Message: 3
Date: Fri, 12 Feb 2016 11:11:38 +0200
From: Patrik Flykt <[email protected]>
To: Naveen Singh <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Sun, 2016-02-07 at 00:06 -0800, Naveen Singh wrote:
> From: nasingh <[email protected]>
>
> Connman did not call netwok_remove in case AP deauthenticated client causing
> wpa_s to re-allocate the ssid pointer even if the next connection attempt
> is for the same SSID. This change ensures that at the time of connection
> (DBUS Method call AddNetwork) if the network is found not removed, it calls
> the dbus API to remove the network and once network is removed, proceed with
> the connection.
>
> Tested by running a deauth loop script at the AP end and ensure that wpa_s
> does
> not allocate memory for struct wpa_ssid for all the subsequent
> connection attempts. During the test memory usage of wpa_s is monitored.
> ---
> gsupplicant/supplicant.c | 139
> ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 89 insertions(+), 50 deletions(-)
>
> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> index 342cb01..b954e91 100644
> --- a/gsupplicant/supplicant.c
> +++ b/gsupplicant/supplicant.c
> @@ -249,6 +249,51 @@ struct _GSupplicantGroup {
> GSList *members;
> };
>
> +struct interface_data {
> + GSupplicantInterface *interface;
> + char *path; /* Interface path cannot be taken from interface (above) as
> + * it might have been freed already.
> + */
> + GSupplicantInterfaceCallback callback;
> + void *user_data;
> + bool network_remove_in_progress;
> +};
> +
> +struct interface_create_data {
> + char *ifname;
> + char *driver;
> + char *bridge;
> + GSupplicantInterface *interface;
> + GSupplicantInterfaceCallback callback;
> + void *user_data;
> +};
> +
> +struct interface_connect_data {
> + GSupplicantInterface *interface;
> + char *path;
> + GSupplicantInterfaceCallback callback;
> + void *user_data;
> + bool network_remove_in_progress;
> + union {
> + GSupplicantSSID *ssid;
> + GSupplicantPeerParams *peer;
> + };
> +};
> +
> +struct interface_scan_data {
> + GSupplicantInterface *interface;
> + char *path;
> + GSupplicantInterfaceCallback callback;
> + GSupplicantScanParams *scan_params;
> + void *user_data;
> +};
> +
> +
> +static int network_remove(struct interface_data *data);
> +static void network_remove_params(DBusMessageIter *iter, void *user_data);
> +static void network_remove_result(const char *error,
> + DBusMessageIter *iter, void *user_data);
> +
> static inline void debug(const char *format, ...)
> {
> char str[256];
> @@ -3476,43 +3521,6 @@ GSupplicantPeer
> *g_supplicant_interface_peer_lookup(GSupplicantInterface *interf
> return peer;
> }
>
> -struct interface_data {
> - GSupplicantInterface *interface;
> - char *path; /* Interface path cannot be taken from interface (above) as
> - * it might have been freed already.
> - */
> - GSupplicantInterfaceCallback callback;
> - void *user_data;
> -};
> -
> -struct interface_create_data {
> - char *ifname;
> - char *driver;
> - char *bridge;
> - GSupplicantInterface *interface;
> - GSupplicantInterfaceCallback callback;
> - void *user_data;
> -};
> -
> -struct interface_connect_data {
> - GSupplicantInterface *interface;
> - char *path;
> - GSupplicantInterfaceCallback callback;
> - union {
> - GSupplicantSSID *ssid;
> - GSupplicantPeerParams *peer;
> - };
> - void *user_data;
> -};
> -
> -struct interface_scan_data {
> - GSupplicantInterface *interface;
> - char *path;
> - GSupplicantInterfaceCallback callback;
> - GSupplicantScanParams *scan_params;
> - void *user_data;
> -};
> -
> static void interface_create_data_free(struct interface_create_data *data)
> {
> g_free(data->ifname);
> @@ -4105,7 +4113,6 @@ static void interface_add_network_result(const char
> *error,
>
> SUPPLICANT_DBG("PATH: %s", path);
>
> - g_free(interface->network_path);
> interface->network_path = g_strdup(path);
Don't we now have a potential memory leak?
> supplicant_dbus_method_call(data->interface->path,
> @@ -4656,7 +4663,7 @@ int g_supplicant_interface_connect(GSupplicantInterface
> *interface,
> void *user_data)
> {
> struct interface_connect_data *data;
> - int ret;
> + int ret = 0;
>
> if (!interface)
> return -EINVAL;
> @@ -4685,12 +4692,33 @@ int
> g_supplicant_interface_connect(GSupplicantInterface *interface,
> SUPPLICANT_INTERFACE ".Interface.WPS",
> "ProcessCredentials", DBUS_TYPE_BOOLEAN_AS_STRING,
> wps_process_credentials, wps_start, data, interface);
> - } else
> - ret = supplicant_dbus_method_call(interface->path,
> - SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
> - interface_add_network_params,
> - interface_add_network_result, data,
> - interface);
> + } else {
> + /* By the time there is a request for connect and the network
> + * path is not NULL it means that connman has not removed the
> + * previous network pointer. This can happen in the case AP
> + * deauthenticated client and connman does not remove the
> + * previously connected network pointer. This causes supplicant
> + * to reallocate the memory for struct wpa_ssid again even if it
> + * is the same SSID. This causes memory usage of wpa_supplicnat
> + * to go high. The idea here is that if the previously connected
> + * network is not removed at the time of next connection attempt
> + * check if the network path is not NULL. In case it is non-NULL
> + * first remove the network and then once removal is
> successful, add
> + * the network.
> + */
> +
> + if (interface->network_path != NULL) {
> + data->network_remove_in_progress = TRUE;
> + network_remove((struct interface_data *)data);
With 'data' being of type struct interface_connect_data, this cast is
very dangerous. It may work for a time, until someone makes changes to
the data structures and then...
Is there any possibility that interface->network_path is already
pointing to the correct network and not the previous one? So that we
don't unnecessary remove the network just to add it again? Is the easy
way out to check that data->path != interface->network_path?
If the network was disconnected with -ECONNABORTED in
interface_disconnect_result(), do we need to still call NetworkRemove?
Same applies to signal_network_removed().
> + }
> + else {
On the same line, please.
> + ret = supplicant_dbus_method_call(interface->path,
> + SUPPLICANT_INTERFACE ".Interface",
> "AddNetwork",
> + interface_add_network_params,
> + interface_add_network_result, data,
> + interface);
> + }
> + }
>
> if (ret < 0) {
> g_free(data->path);
> @@ -4716,12 +4744,23 @@ static void network_remove_result(const char *error,
> result = -ECONNABORTED;
> }
>
> - g_free(data->path);
> + g_free(data->interface->network_path);
> + data->interface->network_path = NULL;
>
> - if (data->callback)
> - data->callback(result, data->interface, data->user_data);
> -
> - dbus_free(data);
> + if (data->network_remove_in_progress == TRUE) {
> + data->network_remove_in_progress = FALSE;
> + supplicant_dbus_method_call(data->interface->path,
> + SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
> + interface_add_network_params,
> + interface_add_network_result, data,
> + data->interface);
> + }
> + else {
> + g_free(data->path);
> + if (data->callback)
> + data->callback(result, data->interface,
> data->user_data);
> + dbus_free(data);
> + }
> }
>
> static void network_remove_params(DBusMessageIter *iter, void *user_data)
Patrik
------------------------------
Message: 4
Date: Fri, 12 Feb 2016 11:16:19 +0200
From: Patrik Flykt <[email protected]>
To: Naveen Singh <[email protected]>
Cc: [email protected]
Subject: Re: Fwd: [PATCH] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Wed, 2016-02-10 at 11:08 -0800, Naveen Singh wrote:
> Hi All
> Looks like this was lost.
Severe case of -EBUSY here. Sorry!
Patrik
------------------------------
Message: 5
Date: Fri, 12 Feb 2016 11:27:42 +0200
From: Patrik Flykt <[email protected]>
To: Saurav Babu <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH] gdhcp: Don't send DHCPREQUEST if last assigned IP
is Link Local Address
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Wed, 2016-02-03 at 19:41 +0530, Saurav Babu wrote:
> In the following scenario:
> 1. connman is connected to a service and Link Local Address is obtained.
> 2. Disconnect the service.
> 3. Connect the service again.
> connman tries to send DHCPREQUEST with last assigned IP which was Link
> Local Address.
> This patch makes connman to send DHCPDISCOVER when last assigned IP was
> Link Local Address.
Applied, thanks!
Patrik
------------------------------
Message: 6
Date: Fri, 12 Feb 2016 11:28:09 +0200
From: Patrik Flykt <[email protected]>
To: Saurav Babu <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH] dhcpv6: Fix memory leak if for loop breaks
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Thu, 2016-02-11 at 22:12 +0530, Saurav Babu wrote:
> This patch duplicates string only when it is required so that there is
> no memory leak when for loop breaks.
Applied, thanks!
Patrik
------------------------------
Message: 7
Date: Fri, 12 Feb 2016 11:28:44 +0200
From: Patrik Flykt <[email protected]>
To: Saurav Babu <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: [PATCH] core: No need to compare expression against NULL
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Tue, 2016-02-09 at 16:10 +0530, Saurav Babu wrote:
> doc/coding-style.txt M13: Check for pointer being NULL
Applied, thanks!
Patrik
------------------------------
Message: 8
Date: Fri, 12 Feb 2016 13:16:21 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [RFC v2] connection: Move online service to ready if gateway
is removed
Message-ID:
<[email protected]>
When the default gateway is removed, move the service to state ready
if the respective ipconfig was connected.
Based on a patch by Naveen Singh.
---
Coming back to the issue, downgrading the state to ready when removing the
gateway does seem the most stable thing to do. This location to do the state
transition was earlier proposed by Naveen Singh. In this variant the ipconfig
state is set to ready only if it is already connected.
It would be better to listen on the default routes added and removed
when setting the state, but this has quite a few implications on testing.
So let's try with this as a much easier working solution.
Please test,
Patrik
src/connection.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/connection.c b/src/connection.c
index 6b005e7..5234043 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -963,6 +963,11 @@ void __connman_connection_gateway_remove(struct
connman_service *service,
if (data)
set_default_gateway(data, type);
}
+
+ if (__connman_service_is_connected_state(service, type))
+ __connman_service_ipconfig_indicate_state(service,
+ CONNMAN_SERVICE_STATE_READY,
+ type);
}
bool __connman_connection_update_gateway(void)
--
2.1.4
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 15
**************************************
