net: lib: download_client: Fix address parsing

Fixed few address parsing issues from download client * If unsupported protocol was used, for example telnet://example.com parser just assumed that parsing failure as no protocol and defaulted to HTTP. Now returns EPROTONOSUPPORT as documented earlier. * If no protocol was provided, but beginning of of the host address matched some protocol, that protocol was assumed. For example: download_client_get(&c, "coap.me/index.html", &cfg , 0, 0) uses coap://, because "coap" matches, but it should have used "http://". Now it needs to have "://" in the protocol to match. Also, converted ztests to native_sim instead of native_posix. Added few more unittests for the cases above. Signed-off-by: Seppo Takalo <[email protected]>
nrfconnect · Apr 2, 2024 · 456af0c · 456af0c
1 parent 283de53
commit 456af0c
Show file tree

Hide file tree

Showing 8 changed files with 115 additions and 28 deletions.
diff --git a/include/net/download_client.h b/include/net/download_client.h
@@ -53,6 +53,7 @@ enum download_client_evt_id {
 	 * - EPROTONOSUPPORT: Protocol is not supported
 	 * - EINVAL: Invalid configuration
 	 * - EAFNOSUPPORT: Unsupported address family (IPv4/IPv6)
+	 * - EHOSTUNREACH: Failed to resolve the target address
 	 *
 	 * In case of errors on the socket during send() or recv() (ECONNRESET),
 	 * returning zero from the callback will let the library attempt

diff --git a/subsys/net/lib/download_client/src/download_client.c b/subsys/net/lib/download_client/src/download_client.c
@@ -385,14 +385,16 @@ static int client_connect(struct download_client *dl)
 	uint16_t port;
 
 	err = url_parse_proto(dl->host, &dl->proto, &type);
-	if (err) {
+	if (err == -EINVAL) {
 		LOG_DBG("Protocol not specified, defaulting to HTTP(S)");
 		type = SOCK_STREAM;
 		if (dl->config.sec_tag_list && (dl->config.sec_tag_count > 0)) {
 			dl->proto = IPPROTO_TLS_1_2;
 		} else {
 			dl->proto = IPPROTO_TCP;
 		}
+	} else if (err) {
+		goto cleanup;
 	}
 
 	if (dl->proto == IPPROTO_UDP || dl->proto == IPPROTO_DTLS_1_2) {

diff --git a/subsys/net/lib/download_client/src/parse.c b/subsys/net/lib/download_client/src/parse.c
@@ -25,18 +25,20 @@ static int swallow(const char **str, const char *swallow)
 
 int url_parse_proto(const char *url, int *proto, int *type)
 {
-	if (strncmp(url, "https", 5) == 0) {
+	if (strncmp(url, "https://", 8) == 0) {
 		*proto = IPPROTO_TLS_1_2;
 		*type = SOCK_STREAM;
-	} else if (strncmp(url, "http", 4) == 0) {
+	} else if (strncmp(url, "http://", 7) == 0) {
 		*proto = IPPROTO_TCP;
 		*type = SOCK_STREAM;
-	} else if (strncmp(url, "coaps", 5) == 0) {
+	} else if (strncmp(url, "coaps://", 8) == 0) {
 		*proto = IPPROTO_DTLS_1_2;
 		*type = SOCK_DGRAM;
-	} else if (strncmp(url, "coap", 4) == 0) {
+	} else if (strncmp(url, "coap://", 7) == 0) {
 		*proto = IPPROTO_UDP;
 		*type = SOCK_DGRAM;
+	} else if (strstr(url, "://")) {
+		return -EPROTONOSUPPORT;
 	} else {
 		return -EINVAL;
 	}

diff --git a/tests/subsys/net/lib/download_client/boards/native_posix.conf b/tests/subsys/net/lib/download_client/boards/native_posix.conf
diff --git a/tests/subsys/net/lib/download_client/boards/native_sim.conf b/tests/subsys/net/lib/download_client/boards/native_sim.conf
@@ -0,0 +1,5 @@
+CONFIG_ASAN=y
+CONFIG_NET_TCP=y
+CONFIG_NET_TCP_ISN_RFC6528=n
+CONFIG_NET_UDP=y
+CONFIG_MBEDTLS=n
diff --git a/tests/subsys/net/lib/download_client/src/main.c b/tests/subsys/net/lib/download_client/src/main.c
@@ -15,6 +15,16 @@
 
 K_PIPE_DEFINE(event_pipe, 10, _Alignof(struct download_client_evt));
 static struct download_client client;
+static const struct sockaddr_in addr_coap_me_http = {
+	.sin_family = AF_INET,
+	.sin_port = htons(80),
+	.sin_addr.s4_addr = {134, 102, 218, 18},
+};
+static const struct sockaddr_in addr_example_com = {
+	.sin_family = AF_INET,
+	.sin_port = htons(5683),
+	.sin_addr.s4_addr = {93, 184, 216, 34},
+};
 
 static int download_client_callback(const struct download_client_evt *event)
 {
@@ -90,7 +100,7 @@ static void init(void)
 
 static void dl_coap_start(void)
 {
-	static const char host[] = "coap://10.1.0.10";
+	static const char host[] = "coap://example.com";
 	int err;
 
 	init();
@@ -110,7 +120,8 @@ ZTEST(download_client, test_download_simple)
 	int32_t recvfrom_params[] = {25, 25, 25};
 	int32_t sendto_params[] = {20, 20, 20};
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
@@ -127,7 +138,8 @@ ZTEST(download_client, test_download_simple)
 
 ZTEST(download_client, test_connection_failed)
 {
-	z_ztest_returns_value("mock_socket_offload_connect", ENETUNREACH);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, ENETUNREACH);
 
 	dl_coap_init(75, 20);
 
@@ -141,29 +153,79 @@ ZTEST(download_client, test_connection_failed)
 	de_init(&client);
 }
 
+ZTEST(download_client, test_getaddr_failed)
+{
+	int err;
+	struct download_client_evt evt;
+
+	init();
+
+	err = download_client_get(&client, "http://unknown_domain.com/file..bin", &config, NULL, 0);
+	zassert_ok(err, NULL);
+	evt = get_next_event(K_FOREVER);
+
+	zassert_equal(evt.id, DOWNLOAD_CLIENT_EVT_ERROR);
+	zassert_equal(evt.error, -EHOSTUNREACH);
+
+	de_init(&client);
+}
+
+ZTEST(download_client, test_default_proto_http)
+{
+	int err;
+	struct download_client_evt evt;
+
+	init();
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_coap_me_http);
+	ztest_returns_value(mock_socket_offload_connect, EHOSTUNREACH);
+	err = download_client_get(&client, "coap.me/file..bin", &config, NULL, 0);
+	zassert_ok(err, NULL);
+	evt = get_next_event(K_FOREVER);
+
+	zassert_equal(evt.id, DOWNLOAD_CLIENT_EVT_ERROR);
+	zassert_equal(evt.error, -EHOSTUNREACH);
+
+	de_init(&client);
+}
+
 ZTEST(download_client, test_wrong_address)
 {
 	int err;
+	struct download_client_evt evt;
 
 	init();
 
+	/* No host or config */
 	err = download_client_get(&client, NULL, NULL, NULL, 0);
 	zassert_equal(err, -EINVAL);
 
+	/* Host, but no config */
 	err = download_client_get(&client, "host", NULL, NULL, 0);
 	zassert_equal(err, -EINVAL);
 
+	/* Try twice to see client does not go into unusable mode */
 	err = download_client_get(&client, "host", NULL, NULL, 0);
 	zassert_equal(err, -EINVAL);
 
+	/* Correct host+config buf fails to resolve */
 	err = download_client_get(&client, "host", &config, NULL, 0);
 	zassert_equal(err, 0);
-	wait_for_event(DOWNLOAD_CLIENT_EVT_ERROR, K_SECONDS(1));
+	evt = wait_for_event(DOWNLOAD_CLIENT_EVT_ERROR, K_SECONDS(1));
+	zassert_equal(evt.error, -EHOSTUNREACH);
 	de_init(&client);
 
+	/* IP that fails to convert */
 	err = download_client_get(&client, "0.0.a", &config, NULL, 0);
 	zassert_equal(err, 0);
-	wait_for_event(DOWNLOAD_CLIENT_EVT_ERROR, K_SECONDS(1));
+	evt = wait_for_event(DOWNLOAD_CLIENT_EVT_ERROR, K_SECONDS(1));
+	zassert_equal(evt.error, -EHOSTUNREACH);
+	de_init(&client);
+
+	/* Unsupported protocol */
+	err = download_client_get(&client, "telnet://192.0.0.1/file.bin", &config, NULL, 0);
+	zassert_equal(err, 0);
+	evt = wait_for_event(DOWNLOAD_CLIENT_EVT_ERROR, K_SECONDS(1));
+	zassert_equal(evt.error, -EPROTONOSUPPORT);
 	de_init(&client);
 
 }
@@ -174,8 +236,10 @@ ZTEST(download_client, test_download_reconnect_on_socket_error)
 	int32_t sendto_params[] = {20, 20, 20, 20};
 	struct download_client_evt evt;
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
@@ -201,8 +265,10 @@ ZTEST(download_client, test_download_reconnect_on_peer_close)
 	int32_t sendto_params[] = {20, 20, 20, 20};
 	struct download_client_evt evt;
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
@@ -231,7 +297,8 @@ ZTEST(download_client, test_download_ignore_duplicate_block)
 	int32_t coap_parse_retv[] = {0, 0, 1, 0};
 	struct download_client_evt evt;
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
@@ -258,7 +325,8 @@ ZTEST(download_client, test_download_abort_on_invalid_block)
 	int32_t sendto_params[] = {20, 20, 20};
 	int32_t coap_parse_retv[] = {0, 0, -1};
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
@@ -281,15 +349,16 @@ ZTEST(download_client, test_get)
 	int32_t recvfrom_params[] = {25, 25, 25};
 	int32_t sendto_params[] = {20, 20, 20};
 
-	z_ztest_returns_value("mock_socket_offload_connect", 0);
+	ztest_expect_data(mock_socket_offload_connect, addr, &addr_example_com);
+	ztest_returns_value(mock_socket_offload_connect, 0);
 
 	dl_coap_init(75, 20);
 
 	mock_return_values("mock_socket_offload_recvfrom", recvfrom_params,
 			   ARRAY_SIZE(recvfrom_params));
 	mock_return_values("mock_socket_offload_sendto", sendto_params, ARRAY_SIZE(sendto_params));
 
-	err = download_client_get(&client, "coap://192.168.1.2/large", &config, NULL, 0);
+	err = download_client_get(&client, "coap://example.com/large", &config, NULL, 0);
 	zassert_ok(err, NULL);
 	wait_for_event(DOWNLOAD_CLIENT_EVT_DONE, K_SECONDS(1));
 	wait_for_event(DOWNLOAD_CLIENT_EVT_CLOSED, K_SECONDS(1));

diff --git a/tests/subsys/net/lib/download_client/src/mock/socket.c b/tests/subsys/net/lib/download_client/src/mock/socket.c
@@ -91,6 +91,7 @@ static int mock_socket_offload_bind(void *obj, const struct sockaddr *addr, sock
 
 static int mock_socket_offload_connect(void *obj, const struct sockaddr *addr, socklen_t addrlen)
 {
+	ztest_check_expected_data(addr, addrlen);
 	k_sleep(K_MSEC(50));
 	errno = ztest_get_return_value();
 	return errno ? -1 : 0;
@@ -203,17 +204,25 @@ static int mock_socket_offload_getaddrinfo(const char *node, const char *service
 
 	ai_addr->sin_family = ai->ai_family;
 	ai_addr->sin_port = htons(port);
+	ai->ai_addrlen = sizeof(*ai_addr);
+	ai->ai_addr = (struct sockaddr *)ai_addr;
 
-	if (!net_ipaddr_parse(node, strlen(node), (struct sockaddr *)ai_addr)) {
-		free(ai_addr);
-		free(*res);
-		return -1;
+	if (net_ipaddr_parse(node, strlen(node), (struct sockaddr *)ai_addr)) {
+		return 0;
+	}
+	if (strncmp(node, "example.com", 11) == 0) {
+		ai_addr->sin_addr = (struct in_addr){.s4_addr = {93, 184, 216, 34}};
+		return 0;
+	}
+	if (strncmp(node, "coap.me", 7) == 0) {
+		ai_addr->sin_addr = (struct in_addr){.s4_addr = {134, 102, 218, 18}};
+		return 0;
 	}
 
-	ai->ai_addrlen = sizeof(*ai_addr);
-	ai->ai_addr = (struct sockaddr *)ai_addr;
 
-	return 0;
+	free(ai_addr);
+	free(*res);
+	return -1;
 }
 
 static void mock_socket_offload_freeaddrinfo(struct zsock_addrinfo *res)

diff --git a/tests/subsys/net/lib/download_client/testcase.yaml b/tests/subsys/net/lib/download_client/testcase.yaml
@@ -1,6 +1,6 @@
 tests:
   net.lib.download_client:
     tags: fota
-    platform_allow: native_posix nrf9160dk_nrf9160 nrf9160dk_nrf9160_ns
+    platform_allow: native_sim nrf9160dk_nrf9160 nrf9160dk_nrf9160_ns
     integration_platforms:
-      - native_posix
+      - native_sim