diff mbox series

[Openvpn-devel,v2] buffer: Clean up rm_trailing_chars

Message ID 20261008161329.32304-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v2] buffer: Clean up rm_trailing_chars |

Commit Message

Gert Doering Oct. 8, 2026, 4:13 p.m. UTC 
  From: Frank Lichtenheld <frank@lichtenheld.com>

This is very weird code. We re-run strlen for
every char that we delete, even though we actually
know which char to test next. Avoid this.

Also add unit tests to make sure I did not break it.

Change-Id: I47e0741c75832a8f3fe161769feb27ddbfba3cdd
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1987
---

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/+/1987
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <razvanc@mailbox.org>
diff mbox series

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 8f558b1..a90ddae 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -592,21 +592,11 @@ 
 void
 rm_trailing_chars(char *str, const char *what_to_delete)
 {
-    bool modified;
-    do
+    size_t len = strlen(str);
+    while (len > 0 && strchr(what_to_delete, str[len - 1]) != NULL)
     {
-        const size_t len = strlen(str);
-        modified = false;
-        if (len > 0)
-        {
-            char *cp = str + (len - 1);
-            if (strchr(what_to_delete, *cp) != NULL)
-            {
-                *cp = '\0';
-                modified = true;
-            }
-        }
-    } while (modified);
+        str[--len] = '\0';
+    }
 }
 
 /*
diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c
index 2945e8a..bf486e0 100644
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -435,6 +435,44 @@ 
 }
 
 void
+test_string_chomp(void **state)
+{
+    char string[10];
+
+    strcpy(string, "Foo\n");
+    chomp(string);
+    assert_string_equal(string, "Foo");
+
+    strcpy(string, "Foo\r\n\n\r");
+    chomp(string);
+    assert_string_equal(string, "Foo");
+
+    strcpy(string, "Foo");
+    chomp(string);
+    assert_string_equal(string, "Foo");
+
+    strcpy(string, "F\ro\no");
+    chomp(string);
+    assert_string_equal(string, "F\ro\no");
+
+    string[2] = '\0';
+    chomp(string);
+    assert_string_equal(string, "F");
+
+    strcpy(string, "");
+    chomp(string);
+    assert_string_equal(string, "");
+
+    strcpy(string, "Foo");
+    rm_trailing_chars(string, "o");
+    assert_string_equal(string, "F");
+
+    strcpy(string, "Foo");
+    rm_trailing_chars(string, "oF");
+    assert_string_equal(string, "");
+}
+
+void
 test_buffer_chomp(void **state)
 {
     struct gc_arena gc = gc_new();
@@ -616,6 +654,7 @@ 
         cmocka_unit_test(test_character_string_mod_buf),
         cmocka_unit_test(test_snprintf),
         cmocka_unit_test(test_checked_snprintf),
+        cmocka_unit_test(test_string_chomp),
         cmocka_unit_test(test_buffer_chomp),
         cmocka_unit_test(test_buffer_null_terminate),
         cmocka_unit_test(test_buffer_null_predicates),