Attention is currently required from: plaisthos. Hello plaisthos,
I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/474?usp=email to look at the new patch set (#2). Change subject: test_user_pass: Check fatal errors for empty username/password ...................................................................... test_user_pass: Check fatal errors for empty username/password Required a fix to mock_msg to make tests of M_FATAL possible at all. This also tests some cases which arguably should throw a fatal error but do not. v2: - Suppress LeakSanitizer errors for fatal error tests. Due to aborting the function, the memory will not be cleaned up, but that is expected. Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97 Signed-off-by: Frank Lichtenheld <fr...@lichtenheld.com> --- M .github/workflows/build.yaml M CMakeLists.txt M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/input/appears_empty.txt A tests/unit_tests/openvpn/input/empty.txt A tests/unit_tests/openvpn/input/leak_suppr.txt M tests/unit_tests/openvpn/mock_msg.c M tests/unit_tests/openvpn/test_user_pass.c 8 files changed, 100 insertions(+), 3 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/474/2 diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index d7cc01c..784a844 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -283,6 +283,7 @@ configurePreset: win-${{ matrix.arch }}-release buildPreset: win-${{ matrix.arch }}-release testPreset: win-${{ matrix.arch }}-release + testPresetAdditionalArgs: "['--output-on-failure']" - uses: actions/upload-artifact@v3 with: diff --git a/CMakeLists.txt b/CMakeLists.txt index 3f96354..142bde4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -621,8 +621,9 @@ if (NOT ${test_name} STREQUAL "test_networking") add_test(${test_name} ${test_name}) # for compat with autotools make check + set(_UT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/tests/unit_tests/openvpn) set_tests_properties(${test_name} PROPERTIES - ENVIRONMENT "srcdir=${CMAKE_CURRENT_SOURCE_DIR}/tests/unit_tests/openvpn") + ENVIRONMENT "srcdir=${_UT_SOURCE_DIR};LSAN_OPTIONS=suppressions=${_UT_SOURCE_DIR}/input/leak_suppr.txt") endif () add_executable(${test_name} tests/unit_tests/openvpn/${test_name}.c diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 6199b4a..ac85aca 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -2,6 +2,8 @@ EXTRA_DIST = input +export LSAN_OPTIONS=suppressions=$(srcdir)/input/leak_suppr.txt + test_binaries= if HAVE_LD_WRAP_SUPPORT diff --git a/tests/unit_tests/openvpn/input/appears_empty.txt b/tests/unit_tests/openvpn/input/appears_empty.txt new file mode 100644 index 0000000..ffb749a --- /dev/null +++ b/tests/unit_tests/openvpn/input/appears_empty.txt @@ -0,0 +1,3 @@ + + +(contains \t\n\t\n) diff --git a/tests/unit_tests/openvpn/input/empty.txt b/tests/unit_tests/openvpn/input/empty.txt new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/tests/unit_tests/openvpn/input/empty.txt diff --git a/tests/unit_tests/openvpn/input/leak_suppr.txt b/tests/unit_tests/openvpn/input/leak_suppr.txt new file mode 100644 index 0000000..72ebfe0 --- /dev/null +++ b/tests/unit_tests/openvpn/input/leak_suppr.txt @@ -0,0 +1 @@ +leak:_assertions$ diff --git a/tests/unit_tests/openvpn/mock_msg.c b/tests/unit_tests/openvpn/mock_msg.c index d74efaa..2fcad9d 100644 --- a/tests/unit_tests/openvpn/mock_msg.c +++ b/tests/unit_tests/openvpn/mock_msg.c @@ -38,7 +38,6 @@ #include "error.h" unsigned int x_debug_level = 0; /* Default to (almost) no debugging output */ -bool fatal_error_triggered = false; void mock_set_debug_level(int level) @@ -58,11 +57,14 @@ { if (flags & M_FATAL) { - fatal_error_triggered = true; printf("FATAL ERROR:"); } vprintf(format, arglist); printf("\n"); + if (flags & M_FATAL) + { + mock_assert(false, "FATAL ERROR", __FILE__, __LINE__); + } } void diff --git a/tests/unit_tests/openvpn/test_user_pass.c b/tests/unit_tests/openvpn/test_user_pass.c index e468d3f..ed65fdb 100644 --- a/tests/unit_tests/openvpn/test_user_pass.c +++ b/tests/unit_tests/openvpn/test_user_pass.c @@ -172,6 +172,16 @@ reset_user_pass(&up); + /*FIXME: query_user_exec() called even though nothing queued */ + will_return(query_user_exec_builtin, true); + /*FIXME: silently removes control characters but does not error out */ + assert_true(get_user_pass_cr(&up, "\t\n\t", "UT", flags, NULL)); + assert_true(up.defined); + assert_string_equal(up.username, ""); + assert_string_equal(up.password, ""); + + reset_user_pass(&up); + expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT Password:"); will_return(query_user_exec_builtin, "cpassword"); will_return(query_user_exec_builtin, true); @@ -205,6 +215,21 @@ } static void +test_get_user_pass_inline_creds_assertions(void **state) +{ + struct user_pass up = { 0 }; + reset_user_pass(&up); + unsigned int flags = GET_USER_PASS_INLINE_CREDS; + + reset_user_pass(&up); + + /*FIXME: query_user_exec() called even though nothing queued */ + /*FIXME? username error thrown very late in stdin handling */ + will_return(query_user_exec_builtin, true); + expect_assert_failure(get_user_pass_cr(&up, "\nipassword\n", "UT", flags, NULL)); +} + +static void test_get_user_pass_authfile_stdin(void **state) { struct user_pass up = { 0 }; @@ -231,6 +256,33 @@ assert_true(up.defined); assert_string_equal(up.username, "user"); assert_string_equal(up.password, "cpassword"); + + reset_user_pass(&up); + + flags |= GET_USER_PASS_PASSWORD_ONLY; + expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT Password:"); + will_return(query_user_exec_builtin, ""); + will_return(query_user_exec_builtin, true); + /*FIXME? does not error out on empty password */ + assert_true(get_user_pass_cr(&up, "stdin", "UT", flags, NULL)); + assert_true(up.defined); + assert_string_equal(up.username, "user"); + assert_string_equal(up.password, ""); +} + +static void +test_get_user_pass_authfile_stdin_assertions(void **state) +{ + struct user_pass up = { 0 }; + reset_user_pass(&up); + unsigned int flags = 0; + + expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT Username:"); + expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT Password:"); + will_return(query_user_exec_builtin, ""); + will_return(query_user_exec_builtin, "cpassword"); + will_return(query_user_exec_builtin, true); + expect_assert_failure(get_user_pass_cr(&up, "stdin", "UT", flags, NULL)); } static void @@ -254,6 +306,17 @@ reset_user_pass(&up); + snprintf(authfile, PATH_MAX, "%s/%s", srcdir, "input/appears_empty.txt"); + /*FIXME: query_user_exec() called even though nothing queued */ + will_return(query_user_exec_builtin, true); + /*FIXME? does not error out */ + assert_true(get_user_pass_cr(&up, authfile, "UT", flags, NULL)); + assert_true(up.defined); + assert_string_equal(up.username, ""); + assert_string_equal(up.password, ""); + + reset_user_pass(&up); + snprintf(authfile, PATH_MAX, "%s/%s", srcdir, "input/user_only.txt"); expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT Password:"); will_return(query_user_exec_builtin, "cpassword"); @@ -276,12 +339,36 @@ assert_string_equal(up.password, "fuser"); } +static void +test_get_user_pass_authfile_file_assertions(void **state) +{ + struct user_pass up = { 0 }; + reset_user_pass(&up); + unsigned int flags = 0; + + const char *srcdir = getenv("srcdir"); + assert_non_null(srcdir); + char authfile[PATH_MAX] = { 0 }; + + snprintf(authfile, PATH_MAX, "%s/%s", srcdir, "input/empty.txt"); + expect_assert_failure(get_user_pass_cr(&up, authfile, "UT", flags, NULL)); + + reset_user_pass(&up); + + flags |= GET_USER_PASS_PASSWORD_ONLY; + snprintf(authfile, PATH_MAX, "%s/%s", srcdir, "input/empty.txt"); + expect_assert_failure(get_user_pass_cr(&up, authfile, "UT", flags, NULL)); +} + const struct CMUnitTest user_pass_tests[] = { cmocka_unit_test(test_get_user_pass_defined), cmocka_unit_test(test_get_user_pass_needok), cmocka_unit_test(test_get_user_pass_inline_creds), + cmocka_unit_test(test_get_user_pass_inline_creds_assertions), cmocka_unit_test(test_get_user_pass_authfile_stdin), + cmocka_unit_test(test_get_user_pass_authfile_stdin_assertions), cmocka_unit_test(test_get_user_pass_authfile_file), + cmocka_unit_test(test_get_user_pass_authfile_file_assertions), }; int -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/474?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97 Gerrit-Change-Number: 474 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <fr...@lichtenheld.com> Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-MessageType: newpatchset
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel