[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2024-05-17 Thread flichtenheld (Code Review) 
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 (#11).


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.
v3:
 - Disable assert tests with MSVC. Does not seem to catch
   the error correctly.
 - Rebase on top of parallel-tests series to get
   AM_TESTS_ENVIRONMENT.
v8:
 - Update srcdir handling according to master.
v10:
 - Update mock_msg.c fatal handling to be compatible
   with NO_CMOCKA.

Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97
Signed-off-by: Frank Lichtenheld 
---
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
7 files changed, 112 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/474/11

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f8b37a9..e32c99c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -668,7 +668,7 @@
 # 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=${_UT_SOURCE_DIR}")
+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 a4e6235..22dc075 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -4,6 +4,8 @@

 AM_TESTSUITE_SUMMARY_HEADER = ' for $(PACKAGE_STRING) Unit-Tests'

+AM_TESTS_ENVIRONMENT = 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 000..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 000..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 000..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 a291f8f..098f56b 100644
--- a/tests/unit_tests/openvpn/mock_msg.c
+++ b/tests/unit_tests/openvpn/mock_msg.c
@@ -39,7 +39,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)
@@ -59,11 +58,16 @@
 {
 if (flags & M_FATAL)
 {
-fatal_error_triggered = true;
 printf("FATAL ERROR:");
 }
 vprintf(format, arglist);
 printf("\n");
+#ifndef NO_CMOCKA
+if (flags & M_FATAL)
+{
+mock_assert(false, "FATAL ERROR", __FILE__, __LINE__);
+}
+#endif
 }

 void
diff --git a/tests/unit_tests/openvpn/test_user_pass.c 
b/tests/unit_tests/openvpn/test_user_pass.c
index ac84654..d6e464f 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -165,6 +165,16 @@

 reset_user_pass();

+/*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(, "\t\n\t", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "");
+assert_string_equal(up.password, "");
+
+reset_user_pass();
+
 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);
@@ -197,6 +207,25 @@
 assert_string_equal(up.password, "cpassword");
 }

+/* NOTE: expect_assert_failure does

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2024-02-07 Thread flichtenheld (Code Review) 
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 (#8).

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


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.
v3:
 - Disable assert tests with MSVC. Does not seem to catch
   the error correctly.
 - Rebase on top of parallel-tests series to get
   AM_TESTS_ENVIRONMENT.
v8:
 - Update srcdir handling according to master.

Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97
Signed-off-by: Frank Lichtenheld 
---
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
7 files changed, 110 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/474/8

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fdd2b01..a96733e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -666,7 +666,7 @@
 # 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=${_UT_SOURCE_DIR}")
+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 88a694d..7b0a086 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -4,6 +4,8 @@

 AM_TESTSUITE_SUMMARY_HEADER = ' for $(PACKAGE_STRING) Unit-Tests'

+AM_TESTS_ENVIRONMENT = 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 000..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 000..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 000..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 5d3f9b6..4506571 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -165,6 +165,16 @@

 reset_user_pass();

+/*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(, "\t\n\t", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "");
+assert_string_equal(up.password, "");
+
+reset_user_pass();
+
 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);
@@ -197,6 +207,25 @@
 assert_string_equal(up.password, "cpassword");
 }

+/* NOTE: expect_assert_failure does not seem to work with

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2024-01-22 Thread flichtenheld (Code Review) 
Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/474?usp=email )

Change subject: test_user_pass: Check fatal errors for empty username/password
..


Patch Set 7:

(1 comment)

Patchset:

PS6:
> This test has a bigger problem with the use of files in the source dir. […]
That has nothing to do with this patch. This problem is introduced and solved 
in https://gerrit.openvpn.net/c/openvpn/+/468. Please discuss there.



--
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: 7
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 22 Jan 2024 13:15:04 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2024-01-19 Thread plaisthos (Code Review) 
Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/474?usp=email )

Change subject: test_user_pass: Check fatal errors for empty username/password
..


Patch Set 6: Code-Review-1

(1 comment)

Patchset:

PS6:
This test has a bigger problem with the use of files in the source dir. In the 
github action run we archive the unit test binaries and run them without the 
source directory present. I am currently facing the same issue in my list test 
that read COPYRIGHT.



--
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: 6
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Fri, 19 Jan 2024 15:44:14 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2023-12-12 Thread flichtenheld (Code Review) 
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 (#3).

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


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.
v3:
 - Disable assert tests with MSVC. Does not seem to catch
   the error correctly.
 - Rebase on top of parallel-tests series to get
   AM_TESTS_ENVIRONMENT.

Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97
Signed-off-by: Frank Lichtenheld 
---
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
7 files changed, 112 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/474/3

diff --git a/CMakeLists.txt b/CMakeLists.txt
index af891a5..bbbf683 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -649,7 +649,7 @@
 # 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=${_UT_SOURCE_DIR}")
+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 342f428..5d95c7b 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -4,6 +4,8 @@

 AM_TESTSUITE_SUMMARY_HEADER = ' for $(PACKAGE_STRING) Unit-Tests'

+AM_TESTS_ENVIRONMENT = 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 000..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 000..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 000..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 277cb1d..d6e5650 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -164,6 +164,16 @@

 reset_user_pass();

+/*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(, "\t\n\t", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "");
+assert_string_equal(up.password, "");
+
+reset_user_pass();
+
 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);
@@ -196,6 +206,25 @@
 assert_string_equal(up.password, "cpassword");
 }

+/* NOTE: expect_assert_failure does not seem to work with MSVC */
+#ifndef _MSC_VER
+/* NOTE: leaks gc memory

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2023-12-11 Thread flichtenheld (Code Review) 
Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/474?usp=email )

Change subject: test_user_pass: Check fatal errors for empty username/password
..


Patch Set 2:

(1 comment)

File tests/unit_tests/openvpn/input/leak_suppr.txt:

http://gerrit.openvpn.net/c/openvpn/+/474/comment/ba78df38_3f4e21f0 :
PS2, Line 1: leak:_assertions$
> This feels unecessarily broad. […]
That was intentional. Having a common suffix for tests that can leak due to 
testing program aborts feels useful. Would you maybe prefer a different suffix? 
Like _leak?



--
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 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 11 Dec 2023 16:06:07 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2023-12-11 Thread plaisthos (Code Review) 
Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/474?usp=email )

Change subject: test_user_pass: Check fatal errors for empty username/password
..


Patch Set 2: Code-Review-1

(2 comments)

Patchset:

PS2:
I tried to replicate the leak that is being supressed (with blanking the 
content of that .txt file) but cannot trigger it at least on macOS. It would be 
also good to actually have a comment in the unit where the supressed leak is 
happening.


File tests/unit_tests/openvpn/input/leak_suppr.txt:

http://gerrit.openvpn.net/c/openvpn/+/474/comment/cf59cf0c_68f7af73 :
PS2, Line 1: leak:_assertions$
This feels unecessarily broad. Especially since we are using it on every unit 
test that is being run and not only on the input unit tests.



--
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 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 11 Dec 2023 14:58:04 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2023-12-08 Thread flichtenheld (Code Review) 
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 
---
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 000..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 000..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 000..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();

+/*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(, "\t\n\t", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "");
+assert_string_equal(up.password, "");
+
+reset_user_pass();
+

[Openvpn-devel] [M] Change in openvpn[master]: test_user_pass: Check fatal errors for empty username/password

2023-12-08 Thread flichtenheld (Code Review) 
Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/474?usp=email

to review the following change.


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.

Change-Id: Icabc8acf75638c86c8c395e9ffecba7a7226cd97
Signed-off-by: Frank Lichtenheld 
---
A tests/unit_tests/openvpn/input/appears_empty.txt
A tests/unit_tests/openvpn/input/empty.txt
M tests/unit_tests/openvpn/mock_msg.c
M tests/unit_tests/openvpn/test_user_pass.c
4 files changed, 68 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/474/1

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 000..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 000..e69de29
--- /dev/null
+++ b/tests/unit_tests/openvpn/input/empty.txt
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..600ec80 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -172,6 +172,24 @@

 reset_user_pass();

+/*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(, "\t\n\t", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "");
+assert_string_equal(up.password, "");
+
+reset_user_pass();
+
+/* Test Assertion */
+/*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(, "\nipassword\n", "UT", flags, 
NULL));
+
+reset_user_pass();
+
 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);
@@ -223,6 +241,15 @@

 reset_user_pass();

+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(, "stdin", "UT", flags, NULL));
+
+reset_user_pass();
+
 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, "cpassword");
@@ -231,6 +258,18 @@
 assert_true(up.defined);
 assert_string_equal(up.username, "user");
 assert_string_equal(up.password, "cpassword");
+
+reset_user_pass();
+
+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(, "stdin", "UT", flags, NULL));
+assert_true(up.defined);
+assert_string_equal(up.username, "user");
+assert_string_equal(up.password, "");
 }

 static void
@@ -254,6 +293,17 @@

 reset_user_pass();

+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(, authfile, "UT",