Merge pull request #925 from pguyot/w44/fix-esp32-getaddrinfo

Fix `net:getaddrinfo/1,2` on ESP32

Fix a memory allocation bug

Optimize memory allocation by using shared maps

Work around an issue of esp-idf's implementation of getaddrinfo that does not
fill protocol and socket type and returns a larger address record

Add an esp32 qemu test for net:getaddrinfo/1,2

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

src/libAtomVM/otp_net.c

@@ -96,9 +96,57 @@ static term eai_errno_to_term(int err, GlobalContext *glb)
     return term_from_int(err);
 }
 
-static inline term make_address(Context *ctx, struct sockaddr_in *addr)
+/**
+ * @brief Make a getaddrino result item as part of the iteration
+ * @param keys pointer to a term to store the keys of the map. If it's
+ *             invalid_term, a non-shared map will be created and the keys term
+ *             will be updated. Otherwise, it's used to create a shared map
+ * @param ai_protocol protocol field of the addrinfo
+ * @param ai_socktype socktype field of the addrinfo
+ * @param inner_addr IP address  that will be stored in both address and addr
+ *             entries of the map
+ * @param global the global context
+ * @returrn the getaddrinfo result item term
+ * @param heap the heap to create terms in, should have sufficient free space
+ * @details This function is called in a loop to create optimized maps that
+ * share keys.
+ * @end
+ */
+static term make_getaddrinfo_result(term *keys, int ai_protocol, int ai_socktype, term inner_addr, GlobalContext *global, Heap *heap)
 {
-    return inet_make_addr4(ntohl(addr->sin_addr.s_addr), &ctx->heap);
+    term result_map;
+    if (term_is_invalid_term(*keys)) {
+        result_map = term_alloc_map(5, heap);
+    } else {
+        result_map = term_alloc_map_maybe_shared(5, *keys, heap);
+    }
+
+    // in the current implementation, this will always be `inet`
+    term family_atom = globalcontext_make_atom(global, ATOM_STR("\x6", "family"));
+    term family = globalcontext_make_atom(global, ATOM_STR("\x4", "inet"));
+    term_set_map_assoc(result_map, 0, family_atom, family);
+
+    term protocol_atom = globalcontext_make_atom(global, ATOM_STR("\x8", "protocol"));
+    term protocol = interop_atom_term_select_atom(protocol_table, ai_protocol, global);
+    term_set_map_assoc(result_map, 1, protocol_atom, term_is_invalid_term(protocol) ? UNDEFINED_ATOM : protocol);
+
+    term type_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "type"));
+    term type = interop_atom_term_select_atom(type_table, ai_socktype, global);
+    term_set_map_assoc(result_map, 2, type_atom, term_is_invalid_term(type) ? UNDEFINED_ATOM : type);
+
+    // embed the inner_addr, but reference it from both address and addr
+    // for compatibility with OTP
+    term address_atom = globalcontext_make_atom(global, ATOM_STR("\x7", "address"));
+    term_set_map_assoc(result_map, 3, address_atom, inner_addr);
+
+    term addr_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "addr"));
+    term_set_map_assoc(result_map, 4, addr_atom, inner_addr);
+
+    if (term_is_invalid_term(*keys)) {
+        *keys = term_get_map_keys(result_map);
+    }
+
+    return result_map;
 }
 
 //
@@ -199,59 +247,69 @@ static term nif_net_getaddrinfo(Context *ctx, int argc, term argv[])
     //          }
     // }]}
     // Note.  We might over-allocate for some more esoteric calls
-    size_t requested_size = TUPLE_SIZE(2) + LIST_SIZE(num_addrinfos, (term_map_size_in_terms(4) + term_map_size_in_terms(3) + TUPLE_SIZE(4)));
+
+    // Determine the number of entries, if we have ai_protocol or ai_socktype as unspec, return two
+    size_t nb_results = 0;
+    size_t requested_size = TUPLE_SIZE(2); // {ok, _}
+    for (struct addrinfo *p = host_info; p != NULL; p = p->ai_next) {
+        // Each list item is:
+        // 1 CONS
+        // 1 IPv4 address
+        // 1 map with 5 items (family, protocol, type, address, addr)
+        // 1 map with 3 items (addr, port, family)
+        requested_size += CONS_SIZE + INET_ADDR4_TUPLE_SIZE;
+        // First result: regular maps
+        // Subsequent results: shared maps
+        if (nb_results) {
+            requested_size += TERM_MAP_SHARED_SIZE(5) + TERM_MAP_SHARED_SIZE(3);
+        } else {
+            requested_size += TERM_MAP_SIZE(5) + TERM_MAP_SIZE(3);
+        }
+        nb_results++;
+        // If protocol or socktype are unspecified (what esp-idf returns), add
+        // another entry so we'll have tcp and udp
+        if (p->ai_protocol == 0 || p->ai_socktype == 0) {
+            nb_results++;
+            // We only need cons and shared maps here as the IP address will be shared
+            requested_size += CONS_SIZE + TERM_MAP_SHARED_SIZE(5) + TERM_MAP_SHARED_SIZE(3);
+        }
+    }
     if (UNLIKELY(memory_ensure_free(ctx, requested_size) != MEMORY_GC_OK)) {
         RAISE_ERROR(OUT_OF_MEMORY_ATOM);
     }
+
     term infos = term_nil();
+    term result_keys = term_invalid_term();
+    term addrinfo_keys = term_invalid_term();
+    term family_atom = globalcontext_make_atom(global, ATOM_STR("\x6", "family"));
+    term inet_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "inet"));
     for (struct addrinfo *p = host_info; p != NULL; p = p->ai_next) {
-
-        // the number of elements in the outer map.  Add 1 because address and addr will point to the same (inner) map
-        // for compatibility with OTP behavior
-        // Cf. https://erlangforums.com/t/discrepancy-in-type-spec-for-net-getaddrinfo/2984
-        int num_elts = (p->ai_family != 0) + (p->ai_protocol != 0) + (p->ai_socktype != 0) + (p->ai_addrlen != 0) + 1;
-        term elt = term_alloc_map(num_elts, &ctx->heap);
-
-        // in the current implementation, this will always be `inet`
-        term family_atom = globalcontext_make_atom(global, ATOM_STR("\x6", "family"));
-        term family = globalcontext_make_atom(global, ATOM_STR("\x4", "inet"));
-        int i = 0;
-        term_set_map_assoc(elt, i++, family_atom, family);
-
-        if (p->ai_protocol) {
-            term protocol_atom = globalcontext_make_atom(global, ATOM_STR("\x8", "protocol"));
-            term protocol = interop_atom_term_select_atom(protocol_table, p->ai_protocol, global);
-            term_set_map_assoc(elt, i++, protocol_atom, term_is_invalid_term(protocol) ? UNDEFINED_ATOM : protocol);
+        term addrinfo_map;
+        if (term_is_invalid_term(addrinfo_keys)) {
+            addrinfo_map = term_alloc_map(3, &ctx->heap);
         } else {
-            TRACE("No protocol defined\n");
+            addrinfo_map = term_alloc_map_maybe_shared(3, addrinfo_keys, &ctx->heap);
         }
-
-        if (p->ai_socktype) {
-            term type_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "type"));
-            term type = interop_atom_term_select_atom(type_table, p->ai_socktype, global);
-            term_set_map_assoc(elt, i++, type_atom, term_is_invalid_term(type) ? UNDEFINED_ATOM : type);
-        } else {
-            TRACE("No socket type defined\n");
+        // The inner addr contains a family, port, and addr
+        term addr_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "addr"));
+        term_set_map_assoc(addrinfo_map, 0, family_atom, inet_atom);
+        term_set_map_assoc(addrinfo_map, 1, PORT_ATOM, term_from_int(port));
+        term address = inet_make_addr4(ntohl(((struct sockaddr_in *) p->ai_addr)->sin_addr.s_addr), &ctx->heap);
+        term_set_map_assoc(addrinfo_map, 2, addr_atom, address);
+
+        if (term_is_invalid_term(addrinfo_keys)) {
+            addrinfo_keys = term_get_map_keys(addrinfo_map);
         }
 
-        if (p->ai_addrlen == sizeof(struct sockaddr_in)) {
-            // The inner addr contains a family, port, and addr
-            term addr_atom = globalcontext_make_atom(global, ATOM_STR("\x4", "addr"));
-            term inner_addr = term_alloc_map(3, &ctx->heap);
-            term_set_map_assoc(inner_addr, 0, family_atom, family);
-            term_set_map_assoc(inner_addr, 1, PORT_ATOM, term_from_int(port));
-            term address = make_address(ctx, (struct sockaddr_in *) p->ai_addr);
-            term_set_map_assoc(inner_addr, 2, addr_atom, address);
-
-            // embed the inner_addr, but reference it from both address and addr
-            // for compatibility with OTP
-            term address_atom = globalcontext_make_atom(global, ATOM_STR("\x7", "address"));
-            term_set_map_assoc(elt, i++, address_atom, inner_addr);
-            term_set_map_assoc(elt, i++, addr_atom, inner_addr);
+        if (p->ai_protocol != 0 && p->ai_socktype != 0) {
+            term result_map = make_getaddrinfo_result(&result_keys, p->ai_protocol, p->ai_socktype, addrinfo_map, ctx->global, &ctx->heap);
+            infos = term_list_prepend(result_map, infos, &ctx->heap);
         } else {
-            TRACE("No address defined\n");
+            term tcp_map = make_getaddrinfo_result(&result_keys, IPPROTO_TCP, SOCK_STREAM, addrinfo_map, ctx->global, &ctx->heap);
+            infos = term_list_prepend(tcp_map, infos, &ctx->heap);
+            term udp_map = make_getaddrinfo_result(&result_keys, IPPROTO_UDP, SOCK_DGRAM, addrinfo_map, ctx->global, &ctx->heap);
+            infos = term_list_prepend(udp_map, infos, &ctx->heap);
         }
-        infos = term_list_prepend(elt, infos, &ctx->heap);
     }
     freeaddrinfo(host_info);
 

src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt

@@ -43,6 +43,7 @@ compile_erlang(test_list_to_binary)
 compile_erlang(test_md5)
 compile_erlang(test_crypto)
 compile_erlang(test_monotonic_time)
+compile_erlang(test_net)
 compile_erlang(test_rtc_slow)
 compile_erlang(test_select)
 compile_erlang(test_socket)
@@ -59,6 +60,7 @@ add_custom_command(
         test_md5.beam
         test_crypto.beam
         test_monotonic_time.beam
+        test_net.beam
         test_rtc_slow.beam
         test_select.beam
         test_socket.beam
@@ -72,6 +74,7 @@ add_custom_command(
         "${CMAKE_CURRENT_BINARY_DIR}/test_md5.beam"
         "${CMAKE_CURRENT_BINARY_DIR}/test_crypto.beam"
         "${CMAKE_CURRENT_BINARY_DIR}/test_monotonic_time.beam"
+        "${CMAKE_CURRENT_BINARY_DIR}/test_net.beam"
         "${CMAKE_CURRENT_BINARY_DIR}/test_rtc_slow.beam"
         "${CMAKE_CURRENT_BINARY_DIR}/test_select.beam"
         "${CMAKE_CURRENT_BINARY_DIR}/test_socket.beam"

src/platforms/esp32/test/main/test_erl_sources/test_net.erl

@@ -0,0 +1,58 @@
+%
+% This file is part of AtomVM.
+%
+% Copyright 2023 Paul Guyot <pguyot@kallisys.net>
+%
+% Licensed under the Apache License, Version 2.0 (the "License");
+% you may not use this file except in compliance with the License.
+% You may obtain a copy of the License at
+%
+%    http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+% See the License for the specific language governing permissions and
+% limitations under the License.
+%
+% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later
+%
+
+-module(test_net).
+-export([start/0]).
+
+start() ->
+    ok = test_service_undefined(),
+    ok = test_service_https(),
+    ok = test_invalid_hostname(),
+    ok.
+
+test_service_undefined() ->
+    % Get address of github.com
+    {ok, Results} = net:getaddrinfo_nif("github.com", undefined),
+    % We should have at least one UDP and one TCP IPv4 entry
+    [_TCPAddr | _] = [
+        Addr
+     || #{addr := Addr, type := stream, protocol := tcp, family := inet} <- Results
+    ],
+    [_UDPAddr | _] = [
+        Addr
+     || #{addr := Addr, type := dgram, protocol := udp, family := inet} <- Results
+    ],
+    ok.
+
+test_service_https() ->
+    {ok, Results} = net:getaddrinfo_nif("github.com", "https"),
+    [_TCPAddr | _] = [
+        Addr
+     || #{addr := Addr, type := stream, protocol := tcp, family := inet} <- Results
+    ],
+    [_UDPAddr | _] = [
+        Addr
+     || #{addr := Addr, type := dgram, protocol := udp, family := inet} <- Results
+    ],
+    ok.
+
+test_invalid_hostname() ->
+    {error, eaifail} = net:getaddrinfo_nif("atomvm.invalid", undefined),
+    ok.

src/platforms/esp32/test/main/test_main.c

@@ -422,10 +422,31 @@ static void got_ip_event_handler(void *arg, esp_event_base_t event_base, int32_t
     network_got_ip = true;
 }
 
+TEST_CASE("test_net", "[test_run]")
+{
+    // esp_netif_init() was called by network_driver_init
+    ESP_LOGI(TAG, "Registering handler\n");
+    network_got_ip = false;
+    ESP_ERROR_CHECK(esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP, &got_ip_event_handler, NULL));
+    ESP_LOGI(TAG, "Starting network\n");
+    esp_netif_t *eth_netif = eth_start();
+
+    while (!network_got_ip) {
+        vTaskDelay(1);
+    }
+
+    term ret_value = avm_test_case("test_net.beam");
+    TEST_ASSERT(ret_value == OK_ATOM);
+
+    ESP_LOGI(TAG, "Stopping network\n");
+    eth_stop(eth_netif);
+}
+
 TEST_CASE("test_socket", "[test_run]")
 {
     // esp_netif_init() was called by network_driver_init
     ESP_LOGI(TAG, "Registering handler\n");
+    network_got_ip = false;
     ESP_ERROR_CHECK(esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP, &got_ip_event_handler, NULL));
     ESP_LOGI(TAG, "Starting network\n");
     esp_netif_t *eth_netif = eth_start();