[Openvpn-devel,v5] openvpnserv: rework ConvertItfDnsDomains and tests

Message ID 20260628130255.25452-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v5] openvpnserv: rework ConvertItfDnsDomains and tests |

Commit Message

Gert Doering June 28, 2026, 1:02 p.m. UTC
  From: Heiko Hund <heiko@ist.eigentlich.net>

Since there were issues with in-place modification of the buffer
repeatedly, re-implement ConvertItfDnsDomains() to use a internal
temporary buffer to prevent use of memmove and the length calculations
that come with it. Code should be easier to grasp since we're dealing
with one set of lengths (WCHARs) instead of two (WCHARs + octets) now.

The unit tests did not actually test the MULTI_SZs correctly, fixed
that and also added some more tests to cover more scenarios.

Change-Id: I8c67633ed3d82a6dc50fbd8fa1af2c50fc45d938
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1730
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1730
This mail reflects revision 5 of this Change.

Acked-by according to Gerrit (reflected above):
Lev Stipakov <lstipakov@gmail.com>
  

Comments

Gert Doering June 29, 2026, 7:38 a.m. UTC | #1
I have only lightly stared at the code, but Lev has thoroughly reviewed
and tested it - and the unit tests have been significantly extended, so
this function should hopefully now be fixed for good, finally.

It was tested via GH Actions which do build and run the unit tests on
windows.

I have extended the commit message a bit to add the Reported-By: line
of the original reporter, plus references to the private GH repo where
the discussion started - not that useful for non-project members, but if
we want to go back in a few years to see "what triggered this patch?"
it might be useful for us.

Lev requested removal of <malloc.h> as it's not needed for this patch
(there are many more malloc() calls already), but Heiko argues that MSDN
"requires both" - so I consider this a bit of future-proofing.

Your patch has been applied to the master and release/2.7 branch.

Release/2.6 and older do not have the "new DNS NRPT magic" code.

commit 294d5ebb32e632c756290cb7b1de3df29a516154 (master)
commit 59113da80f47d61d7064b1d3b2611b052e4f0923 (release/2.7)
Author: Heiko Hund
Date:   Sun Jun 28 15:02:50 2026 +0200

     openvpnserv: rework ConvertItfDnsDomains and tests

     Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1730
     Message-Id: <20260628130255.25452-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37337.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
  

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 788a578..516a469 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -33,6 +33,7 @@ 
 #include <shellapi.h>
 #include <mstcpip.h>
 #include <inttypes.h>
+#include <malloc.h>
 
 #include <versionhelpers.h>
 
@@ -2130,22 +2131,20 @@ 
 static BOOL
 ListContainsDomain(PCWSTR list, PCWSTR domain, size_t len)
 {
-    PCWSTR match = list;
-    while (match)
+    PCWSTR entry = list;
+    while (entry && *entry)
     {
-        match = wcsstr(match, domain);
-        if (!match)
+        PCWSTR comma = wcschr(entry, L',');
+        size_t entry_len = comma ? (size_t)(comma - entry) : wcslen(entry);
+        if (entry_len == len && wcsncmp(entry, domain, len) == 0)
         {
-            /* Domain has not matched */
-            break;
-        }
-        if ((match == list || *(match - 1) == ',')
-            && (*(match + len) == ',' || *(match + len) == '\0'))
-        {
-            /* Domain has matched fully */
             return TRUE;
         }
-        match += len;
+        if (!comma)
+        {
+            break;
+        }
+        entry = comma + 1;
     }
     return FALSE;
 }
@@ -2159,95 +2158,94 @@ 
  * are invalid.
  * Note that domains are deleted from the string if they match a search domain.
  *
- * @param[in]     search_domains  optional list of search domains
+ * @param[in]     search_domains  optional string of comma separated search domains
  * @param[in,out] domains         buffer that contains the input comma-separated
  *                                string and will contain the MULTI_SZ output string
  * @param[in,out] size            pointer to size of the input string in bytes. Will be
  *                                set to the size of the string returned, including
  *                                the terminating zeros or 0.
- * @param[in]     buf_size        size of the \p domains buffer
+ * @param[in]     capacity        capacity of the \p domains buffer in bytes
  *
- * @return LSTATUS NO_ERROR if the domain suffix(es) were read successfully,
- *         ERROR_FILE_NOT_FOUND if no domain was found for the interface,
- *         ERROR_MORE_DATA if the list did not fit into the buffer
+ * @return LSTATUS NO_ERROR if all domain suffix(es) were converted successfully,
+ *         ERROR_FILE_NOT_FOUND if no domain was left after the conversion,
+ *         ERROR_MORE_DATA if not all converted domains did fit into the buffer.
+ *         ERROR_OUTOFMEMORY if the temporary buffer could not be allocated.
  */
 static LSTATUS
-ConvertItfDnsDomains(PCWSTR search_domains, PWSTR domains, PDWORD size, const DWORD buf_size)
+ConvertItfDnsDomains(PCWSTR search_domains, PWSTR domains, PDWORD size, const DWORD capacity)
 {
-    const DWORD glyph_size = sizeof(*domains);
-    const DWORD buf_len = buf_size / glyph_size;
+    const size_t glyph_size = sizeof(*domains);
+    const size_t max_len = (size_t)capacity / glyph_size;
 
-    /*
-     * Found domain(s), now convert them:
-     *   - prefix each domain with a dot
-     *   - convert comma separated list to MULTI_SZ
-     */
-    PWCHAR pos = domains;
-    while (TRUE)
+    /* Space required for leading dot and two terminating zeros */
+    const size_t dot_len = 1;
+    const size_t term_len = 2;
+
+    LSTATUS ret = NO_ERROR;
+    size_t tmp_len = 0;
+    WCHAR *tmp = malloc(capacity);
+    if (tmp == NULL)
     {
-        /* Terminate the domain at the next comma */
-        PWCHAR comma = wcschr(pos, ',');
-        if (comma)
+        ret = ERROR_OUTOFMEMORY;
+        goto done;
+    }
+
+    PWCHAR tmp_pos = tmp;
+    PCWCHAR domain = domains;
+
+    while (domain && *domain)
+    {
+        PWCHAR comma = wcschr(domain, L',');
+        size_t domain_len = comma ? (size_t)(comma - domain) : wcslen(domain);
+
+        if (ListContainsDomain(search_domains, domain, domain_len))
         {
-            *comma = '\0';
+            /* Skip this domain */
+            domain = comma ? comma + 1 : domain + domain_len;
+            continue;
         }
 
-        DWORD domain_len = (DWORD)wcslen(pos);
-        DWORD domain_size = domain_len * glyph_size;
-        DWORD converted_size = (DWORD)(pos - domains) * glyph_size;
-
-        /* Ignore itf domains which match a pushed search domain */
-        if (ListContainsDomain(search_domains, pos, domain_len))
-        {
-            if (comma)
-            {
-                /* Overwrite the ignored domain with remaining one(s) */
-                memmove(pos, comma + 1, buf_size - converted_size);
-                *size -= domain_size + glyph_size;
-                continue;
-            }
-            else
-            {
-                /* This was the last domain */
-                *pos = '\0';
-                *size -= domain_size;
-                return wcslen(domains) ? NO_ERROR : ERROR_FILE_NOT_FOUND;
-            }
-        }
-
-        /* Add space for the leading dot */
-        domain_len += 1;
-        domain_size += glyph_size;
-
-        /* Space for the terminating zeros */
-        const DWORD extra_size = 2 * glyph_size;
-
         /* Check for enough space to convert this domain */
-        if (converted_size + domain_size + extra_size > buf_size)
+        if (tmp_len + dot_len + domain_len + term_len > max_len)
         {
             /* Domain doesn't fit, bad luck if it's the first one */
-            *pos = '\0';
-            *size = converted_size == 0 ? 0 : converted_size + glyph_size;
-            return ERROR_MORE_DATA;
+            *tmp_pos = L'\0';
+            if (tmp_len > 0)
+            {
+                tmp_len += 1;
+            }
+            ret = ERROR_MORE_DATA;
+            goto done;
         }
 
-        /* Prefix domain at pos with the dot */
-        memmove(pos + 1, pos, buf_size - converted_size - glyph_size);
-        domains[buf_len - 1] = '\0';
-        *pos = '.';
-        *size += glyph_size;
+        /* Write leading dot and domain into tmp buffer */
+        *tmp_pos++ = L'.';
+        wcsncpy(tmp_pos, domain, domain_len);
+        tmp_pos += domain_len;
+        *tmp_pos++ = L'\0';
+        tmp_len += dot_len + domain_len + 1;
 
-        if (!comma)
-        {
-            /* Conversion is done */
-            *(pos + domain_len) = '\0';
-            *size += glyph_size;
-            return NO_ERROR;
-        }
-
-        /* Comma pos is now +1 after adding leading dot */
-        pos = comma + 2;
+        domain = comma ? comma + 1 : domain + domain_len;
     }
+
+    if (tmp_len == 0)
+    {
+        ret = ERROR_FILE_NOT_FOUND;
+        goto done;
+    }
+
+    /* REG_MULTI_SZ second zero terminator */
+    *tmp_pos = L'\0';
+    tmp_len += 1;
+
+done:
+    if (tmp)
+    {
+        wmemcpy(domains, tmp, tmp_len);
+        free(tmp);
+    }
+    *size = (DWORD)(tmp_len * glyph_size);
+    return ret;
 }
 
 /**
diff --git a/tests/unit_tests/openvpnserv/test_openvpnserv.c b/tests/unit_tests/openvpnserv/test_openvpnserv.c
index 45096a1..e432f44 100644
--- a/tests/unit_tests/openvpnserv/test_openvpnserv.c
+++ b/tests/unit_tests/openvpnserv/test_openvpnserv.c
@@ -48,58 +48,109 @@ 
     assert_true(ListContainsDomain(domain, domain, domain_len));
     assert_true(ListContainsDomain(L"openvpn.com,openvpn.net", domain, domain_len));
     assert_true(ListContainsDomain(L"openvpn.net,openvpn.com", domain, domain_len));
+    assert_true(ListContainsDomain(L"openvpn.org,openvpn.net,openvpn.com", domain, domain_len));
 
     assert_false(ListContainsDomain(L"openvpn.com", domain, domain_len));
     assert_false(ListContainsDomain(L"internal.openvpn.net", domain, domain_len));
+    assert_false(ListContainsDomain(L"openvpn.com,internal.openvpn.net", domain, domain_len));
+    assert_false(ListContainsDomain(L"internal.openvpn.net,openvpn.com", domain, domain_len));
 }
 
 #define BUF_SIZE 64
 static void
 test_convert_itf_dns_domains(void **state)
 {
-    DWORD size, orig_size, len, res_len;
+    DWORD size, len;
     LSTATUS err;
     const DWORD glyph_size = sizeof(wchar_t);
 
+    /* Remove the domain from a single-entry list */
+    wchar_t domains_0[BUF_SIZE] = L"openvpn.com";
+    len = (DWORD)wcslen(domains_0) + 1;
+    size = len * glyph_size;
+    err = ConvertItfDnsDomains(L"openvpn.com", domains_0, &size, sizeof(domains_0));
+    assert_int_equal(size, 0);
+    assert_int_equal(err, ERROR_FILE_NOT_FOUND);
+
+    /* Remove no domain from a single-entry list */
     wchar_t domains_1[BUF_SIZE] = L"openvpn.com";
     len = (DWORD)wcslen(domains_1) + 1;
-    size = orig_size = len * glyph_size;
-    wchar_t domains_1_res[BUF_SIZE] = L".openvpn.com";
-    res_len = len + 2; /* adds . and \0 */
-    err = ConvertItfDnsDomains(L"openvpn.net", domains_1, &size, BUF_SIZE);
+    size = len * glyph_size;
+    wchar_t domains_1_res[] = L".openvpn.com\0";
+    err = ConvertItfDnsDomains(L"openvpn.net", domains_1, &size, sizeof(domains_1));
     assert_memory_equal(domains_1, domains_1_res, size);
-    assert_int_equal(size, res_len * glyph_size);
+    assert_int_equal(size, sizeof(domains_1_res));
     assert_int_equal(err, NO_ERROR);
 
+    /* Remove the second domain from a two-entry list */
     wchar_t domains_2[BUF_SIZE] = L"openvpn.com,openvpn.net";
     len = (DWORD)wcslen(domains_2) + 1;
-    size = orig_size = len * glyph_size;
-    wchar_t domains_2_res[BUF_SIZE] = L".openvpn.com";
-    res_len = (DWORD)wcslen(domains_2_res) + 2;
-    err = ConvertItfDnsDomains(L"openvpn.net", domains_2, &size, BUF_SIZE);
+    size = len * glyph_size;
+    wchar_t domains_2_res[] = L".openvpn.com\0";
+    err = ConvertItfDnsDomains(L"openvpn.net", domains_2, &size, sizeof(domains_2));
     assert_memory_equal(domains_2, domains_2_res, size);
-    assert_int_equal(size, res_len * glyph_size);
+    assert_int_equal(size, sizeof(domains_2_res));
     assert_int_equal(err, NO_ERROR);
 
+    /* Remove the first domain from a two-entry list */
     wchar_t domains_3[BUF_SIZE] = L"openvpn.com,openvpn.net";
     len = (DWORD)wcslen(domains_3) + 1;
-    size = orig_size = len * glyph_size;
-    wchar_t domains_3_res[BUF_SIZE] = L".openvpn.net";
-    res_len = (DWORD)wcslen(domains_3_res) + 2;
-    err = ConvertItfDnsDomains(L"openvpn.com", domains_3, &size, BUF_SIZE);
+    size = len * glyph_size;
+    wchar_t domains_3_res[] = L".openvpn.net\0";
+    err = ConvertItfDnsDomains(L"openvpn.com", domains_3, &size, sizeof(domains_3));
     assert_memory_equal(domains_3, domains_3_res, size);
-    assert_int_equal(size, res_len * glyph_size);
+    assert_int_equal(size, sizeof(domains_3_res));
     assert_int_equal(err, NO_ERROR);
 
+    /* Remove no domain from a two-entry list */
     wchar_t domains_4[BUF_SIZE] = L"openvpn.com,openvpn.net";
     len = (DWORD)wcslen(domains_4) + 1;
-    size = orig_size = len * glyph_size;
-    wchar_t domains_4_res[BUF_SIZE] = L".openvpn.com\0.openvpn.net";
-    res_len = len + 3; /* adds two . and one \0 */
-    err = ConvertItfDnsDomains(NULL, domains_4, &size, BUF_SIZE);
+    size = len * glyph_size;
+    wchar_t domains_4_res[] = L".openvpn.com\0.openvpn.net\0";
+    err = ConvertItfDnsDomains(NULL, domains_4, &size, sizeof(domains_4));
     assert_memory_equal(domains_4, domains_4_res, size);
-    assert_int_equal(size, res_len * glyph_size);
+    assert_int_equal(size, sizeof(domains_4_res));
     assert_int_equal(err, NO_ERROR);
+
+    /* Remove the first domain from a three-entry list */
+    wchar_t domains_5[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
+    len = (DWORD)wcslen(domains_5) + 1;
+    size = len * glyph_size;
+    wchar_t domains_5_res[] = L".openvpn.net\0.openvpn.org\0";
+    err = ConvertItfDnsDomains(L"openvpn.com", domains_5, &size, sizeof(domains_5));
+    assert_memory_equal(domains_5, domains_5_res, size);
+    assert_int_equal(size, sizeof(domains_5_res));
+    assert_int_equal(err, NO_ERROR);
+
+    /* Remove the middle domain from a three-entry list */
+    wchar_t domains_6[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
+    len = (DWORD)wcslen(domains_6) + 1;
+    size = len * glyph_size;
+    wchar_t domains_6_res[] = L".openvpn.com\0.openvpn.org\0";
+    err = ConvertItfDnsDomains(L"openvpn.net", domains_6, &size, sizeof(domains_6));
+    assert_memory_equal(domains_6, domains_6_res, size);
+    assert_int_equal(size, sizeof(domains_6_res));
+    assert_int_equal(err, NO_ERROR);
+
+    /* Remove the last domain from a three-entry list */
+    wchar_t domains_7[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
+    len = (DWORD)wcslen(domains_7) + 1;
+    size = len * glyph_size;
+    wchar_t domains_7_res[] = L".openvpn.com\0.openvpn.net\0";
+    err = ConvertItfDnsDomains(L"openvpn.org", domains_7, &size, sizeof(domains_7));
+    assert_memory_equal(domains_7, domains_7_res, size);
+    assert_int_equal(size, sizeof(domains_7_res));
+    assert_int_equal(err, NO_ERROR);
+
+    /* Remove the last domain from a four-entry list because of size constraints*/
+    wchar_t domains_8[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org,am-ende-noch-eine-lange.de";
+    len = (DWORD)wcslen(domains_8) + 1;
+    size = len * glyph_size;
+    wchar_t domains_8_res[] = L".openvpn.com\0.openvpn.net\0.openvpn.org\0";
+    err = ConvertItfDnsDomains(NULL, domains_8, &size, sizeof(domains_8));
+    assert_memory_equal(domains_8, domains_8_res, size);
+    assert_int_equal(size, sizeof(domains_8_res));
+    assert_int_equal(err, ERROR_MORE_DATA);
 }
 
 int