[Qemu-devel] [PATCH for 3.0 0/4] Multiple fixes and improvements to TLS tests

[Qemu-devel] [PATCH for 3.0 0/4] Multiple fixes and improvements to TLS tests

[Daniel P. Berrangé]

The switch to enable TLS 1.3 protocol support in GNUTLS in Fedora
exposed a subtle flaw in our TLS unit tests. This was horrible to debug
because of bad error reporting in our tests which caused all the error
messages to be irretrievably lost instead of displayed on stderr.

Daniel P. Berrangé (4):
  tests: call qcrypto_init instead of gnutls_global_init
  tests: don't silence error reporting for all tests
  tests: use error_abort in places expecting errors
  tests: fix TLS handshake failure with TLS 1.3

 stubs/error-printf.c             |  5 +-
 tests/crypto-tls-x509-helpers.c  |  4 +-
 tests/test-crypto-tlscredsx509.c | 11 +----
 tests/test-crypto-tlssession.c   | 80 ++++++++++++--------------------
 tests/test-io-channel-tls.c      | 24 ++++------
 tests/test-vmstate.c             |  3 ++
 6 files changed, 51 insertions(+), 76 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH for 3.0 1/4] tests: call qcrypto_init instead of gnutls_global_init

[Daniel P. Berrangé]

Calling qcrypto_init ensures that all relevant initialization is
done. In particular this honours the debugging settings and thread
settings.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/crypto-tls-x509-helpers.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/crypto-tls-x509-helpers.c b/tests/crypto-tls-x509-helpers.c
index 173d4e28fb..70df68f5df 100644
--- a/tests/crypto-tls-x509-helpers.c
+++ b/tests/crypto-tls-x509-helpers.c
@@ -21,6 +21,8 @@
 #include "qemu/osdep.h"
 
 #include "crypto-tls-x509-helpers.h"
+#include "crypto/init.h"
+#include "qapi/error.h"
 #include "qemu/sockets.h"
 
 #ifdef QCRYPTO_HAVE_TLS_TEST_SUPPORT
@@ -95,7 +97,7 @@ static gnutls_x509_privkey_t test_tls_load_key(void)
 
 void test_tls_init(const char *keyfile)
 {
-    gnutls_global_init();
+    qcrypto_init(&error_abort);
 
     if (asn1_array2tree(pkix_asn1_tab, &pkix_asn1, NULL) != ASN1_SUCCESS) {
         abort();
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.0 1/4] tests: call qcrypto_init instead of gnutls_global_init

[Philippe Mathieu-Daudé]

Hi Daniel,

On 07/18/2018 06:38 AM, Daniel P. Berrangé wrote:
> Calling qcrypto_init ensures that all relevant initialization is
> done. In particular this honours the debugging settings and thread
> settings.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/crypto-tls-x509-helpers.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/crypto-tls-x509-helpers.c b/tests/crypto-tls-x509-helpers.c
> index 173d4e28fb..70df68f5df 100644
> --- a/tests/crypto-tls-x509-helpers.c
> +++ b/tests/crypto-tls-x509-helpers.c
> @@ -21,6 +21,8 @@
>  #include "qemu/osdep.h"
>  
>  #include "crypto-tls-x509-helpers.h"
> +#include "crypto/init.h"
> +#include "qapi/error.h"

This include belongs to "crypto/init.h".

>  #include "qemu/sockets.h"
>  
>  #ifdef QCRYPTO_HAVE_TLS_TEST_SUPPORT
> @@ -95,7 +97,7 @@ static gnutls_x509_privkey_t test_tls_load_key(void)
>  
>  void test_tls_init(const char *keyfile)
>  {
> -    gnutls_global_init();
> +    qcrypto_init(&error_abort);
>  
>      if (asn1_array2tree(pkix_asn1_tab, &pkix_asn1, NULL) != ASN1_SUCCESS) {
>          abort();
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[Qemu-devel] [PATCH for 3.0 2/4] tests: don't silence error reporting for all tests

[Daniel P. Berrangé]

The test-vmstate test is a bit chatty because it triggers various
expected failure scenarios and the code in question uses error_report
instead of accepting 'Error **errp' parameters. To silence this test the
stubs for error_vprintf() were changed to send errors via
g_test_message() instead of stderr:

  commit 28017e010ddf6849cfa830e898da3e44e6610952
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Mon Oct 24 18:31:03 2016 +0200

    tests: send error_report to test log

    Implement error_vprintf to send the output of error_report to
    the test log.  This silences test-vmstate.

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Message-Id: <1477326663-67817-3-git-send-email-pbonzini@redhat.com>

Unfortunately this change has global impact across the entire test suite
and means that when tests fail for unexpected reasons, the message is
not displayed on stderr. eg when using &error_abort in a call the test
merely prints

  Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:

and the actual error message is hidden, making it impossible to diagnose
the failure. This is especially problematic in CI or build systems where
it isn't possible to easily pass the --debug-log flag to tests and
re-run with the test log visible.

This change makes the previous big hammer much more nuanced, providing a
flag in the stub error_vprintf() that can used on a per-test basis to
silence the errors. Only the test-vmstate silences errors initially.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 stubs/error-printf.c | 5 ++++-
 tests/test-vmstate.c | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index ac6b92aa69..2199d79d28 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -2,9 +2,12 @@
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 
+bool silence_test_errors;
+
 void error_vprintf(const char *fmt, va_list ap)
 {
-    if (g_test_initialized() && !g_test_subprocess()) {
+    if (g_test_initialized() && !g_test_subprocess() &&
+        getenv("QTEST_SILENT_ERRORS")) {
         char *msg = g_strdup_vprintf(fmt, ap);
         g_test_message("%s", msg);
         g_free(msg);
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 087844b6c8..42923bb1df 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -32,6 +32,7 @@
 #include "../migration/qemu-file-channel.h"
 #include "../migration/savevm.h"
 #include "qemu/coroutine.h"
+#include "qemu/error-report.h"
 #include "io/channel-file.h"
 
 static char temp_file[] = "/tmp/vmst.test.XXXXXX";
@@ -859,6 +860,8 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
 
+    setenv("QTEST_SILENT_ERRORS", "1", 1);
+
     g_test_init(&argc, &argv, NULL);
     g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
     g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.0 2/4] tests: don't silence error reporting for all tests

[Philippe Mathieu-Daudé]

Hi Daniel,

On 07/18/2018 06:38 AM, Daniel P. Berrangé wrote:
> The test-vmstate test is a bit chatty because it triggers various
> expected failure scenarios and the code in question uses error_report
> instead of accepting 'Error **errp' parameters. To silence this test the
> stubs for error_vprintf() were changed to send errors via
> g_test_message() instead of stderr:
> 
>   commit 28017e010ddf6849cfa830e898da3e44e6610952
>   Author: Paolo Bonzini <pbonzini@redhat.com>
>   Date:   Mon Oct 24 18:31:03 2016 +0200
> 
>     tests: send error_report to test log
> 
>     Implement error_vprintf to send the output of error_report to
>     the test log.  This silences test-vmstate.
> 
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Message-Id: <1477326663-67817-3-git-send-email-pbonzini@redhat.com>
> 
> Unfortunately this change has global impact across the entire test suite
> and means that when tests fail for unexpected reasons, the message is
> not displayed on stderr. eg when using &error_abort in a call the test
> merely prints
> 
>   Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:
> 
> and the actual error message is hidden, making it impossible to diagnose
> the failure. This is especially problematic in CI or build systems where
> it isn't possible to easily pass the --debug-log flag to tests and
> re-run with the test log visible.
> 
> This change makes the previous big hammer much more nuanced, providing a
> flag in the stub error_vprintf() that can used on a per-test basis to
> silence the errors. Only the test-vmstate silences errors initially.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  stubs/error-printf.c | 5 ++++-
>  tests/test-vmstate.c | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> index ac6b92aa69..2199d79d28 100644
> --- a/stubs/error-printf.c
> +++ b/stubs/error-printf.c
> @@ -2,9 +2,12 @@
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
>  
> +bool silence_test_errors;

This is not used.

> +
>  void error_vprintf(const char *fmt, va_list ap)
>  {
> -    if (g_test_initialized() && !g_test_subprocess()) {
> +    if (g_test_initialized() && !g_test_subprocess() &&
> +        getenv("QTEST_SILENT_ERRORS")) {
>          char *msg = g_strdup_vprintf(fmt, ap);
>          g_test_message("%s", msg);
>          g_free(msg);
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 087844b6c8..42923bb1df 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -32,6 +32,7 @@
>  #include "../migration/qemu-file-channel.h"
>  #include "../migration/savevm.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/error-report.h"

Why? This doesn't seem necessary, neither related to this patch.

>  #include "io/channel-file.h"
>  
>  static char temp_file[] = "/tmp/vmst.test.XXXXXX";
> @@ -859,6 +860,8 @@ int main(int argc, char **argv)
>  
>      module_call_init(MODULE_INIT_QOM);
>  
> +    setenv("QTEST_SILENT_ERRORS", "1", 1);
> +
>      g_test_init(&argc, &argv, NULL);
>      g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
>      g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH for 3.0 2/4] tests: don't silence error reporting for all tests

[Daniel P. Berrangé]

On Wed, Jul 18, 2018 at 10:44:11AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> On 07/18/2018 06:38 AM, Daniel P. Berrangé wrote:
> > The test-vmstate test is a bit chatty because it triggers various
> > expected failure scenarios and the code in question uses error_report
> > instead of accepting 'Error **errp' parameters. To silence this test the
> > stubs for error_vprintf() were changed to send errors via
> > g_test_message() instead of stderr:
> > 
> >   commit 28017e010ddf6849cfa830e898da3e44e6610952
> >   Author: Paolo Bonzini <pbonzini@redhat.com>
> >   Date:   Mon Oct 24 18:31:03 2016 +0200
> > 
> >     tests: send error_report to test log
> > 
> >     Implement error_vprintf to send the output of error_report to
> >     the test log.  This silences test-vmstate.
> > 
> >     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >     Message-Id: <1477326663-67817-3-git-send-email-pbonzini@redhat.com>
> > 
> > Unfortunately this change has global impact across the entire test suite
> > and means that when tests fail for unexpected reasons, the message is
> > not displayed on stderr. eg when using &error_abort in a call the test
> > merely prints
> > 
> >   Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:
> > 
> > and the actual error message is hidden, making it impossible to diagnose
> > the failure. This is especially problematic in CI or build systems where
> > it isn't possible to easily pass the --debug-log flag to tests and
> > re-run with the test log visible.
> > 
> > This change makes the previous big hammer much more nuanced, providing a
> > flag in the stub error_vprintf() that can used on a per-test basis to
> > silence the errors. Only the test-vmstate silences errors initially.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  stubs/error-printf.c | 5 ++++-
> >  tests/test-vmstate.c | 3 +++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> > index ac6b92aa69..2199d79d28 100644
> > --- a/stubs/error-printf.c
> > +++ b/stubs/error-printf.c
> > @@ -2,9 +2,12 @@
> >  #include "qemu-common.h"
> >  #include "qemu/error-report.h"
> >  
> > +bool silence_test_errors;
> 
> This is not used.

Yes, accidentally left over from earlier version of patch

> 
> > +
> >  void error_vprintf(const char *fmt, va_list ap)
> >  {
> > -    if (g_test_initialized() && !g_test_subprocess()) {
> > +    if (g_test_initialized() && !g_test_subprocess() &&
> > +        getenv("QTEST_SILENT_ERRORS")) {
> >          char *msg = g_strdup_vprintf(fmt, ap);
> >          g_test_message("%s", msg);
> >          g_free(msg);
> > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> > index 087844b6c8..42923bb1df 100644
> > --- a/tests/test-vmstate.c
> > +++ b/tests/test-vmstate.c
> > @@ -32,6 +32,7 @@
> >  #include "../migration/qemu-file-channel.h"
> >  #include "../migration/savevm.h"
> >  #include "qemu/coroutine.h"
> > +#include "qemu/error-report.h"
> 
> Why? This doesn't seem necessary, neither related to this patch.

Left over from an earlier version of the patch, I'll cull it.

> 
> >  #include "io/channel-file.h"
> >  
> >  static char temp_file[] = "/tmp/vmst.test.XXXXXX";
> > @@ -859,6 +860,8 @@ int main(int argc, char **argv)
> >  
> >      module_call_init(MODULE_INIT_QOM);
> >  
> > +    setenv("QTEST_SILENT_ERRORS", "1", 1);
> > +
> >      g_test_init(&argc, &argv, NULL);
> >      g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
> >      g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);
> > 
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH for 3.0 3/4] tests: use error_abort in places expecting errors

[Daniel P. Berrangé]

Most of the TLS related tests are passing an in a "Error" object to
methods that are expected to fail, but then ignoring any error that is
set and instead asserting on a return value. This means that when an
error is unexpectedly raised, no information about it is printed out,
making failures hard to diagnose. Changing these tests to pass in
&error_abort will make unexpected failures print messages to stderr.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/test-crypto-tlscredsx509.c | 11 +----
 tests/test-crypto-tlssession.c   | 76 ++++++++++++--------------------
 tests/test-io-channel-tls.c      | 24 ++++------
 3 files changed, 39 insertions(+), 72 deletions(-)

diff --git a/tests/test-crypto-tlscredsx509.c b/tests/test-crypto-tlscredsx509.c
index af2f80e89c..30f9ac4bbf 100644
--- a/tests/test-crypto-tlscredsx509.c
+++ b/tests/test-crypto-tlscredsx509.c
@@ -54,7 +54,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         "sanity-check", "yes",
         NULL);
 
-    if (*errp) {
+    if (!creds) {
         return NULL;
     }
     return QCRYPTO_TLS_CREDS(creds);
@@ -74,7 +74,6 @@ static void test_tls_creds(const void *opaque)
     struct QCryptoTLSCredsTestData *data =
         (struct QCryptoTLSCredsTestData *)opaque;
     QCryptoTLSCreds *creds;
-    Error *err = NULL;
 
 #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/"
     mkdir(CERT_DIR, 0700);
@@ -113,17 +112,11 @@ static void test_tls_creds(const void *opaque)
          QCRYPTO_TLS_CREDS_ENDPOINT_SERVER :
          QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT),
         CERT_DIR,
-        &err);
+        data->expectFail ? NULL : &error_abort);
 
     if (data->expectFail) {
-        error_free(err);
         g_assert(creds == NULL);
     } else {
-        if (err) {
-            g_printerr("Failed to generate creds: %s\n",
-                       error_get_pretty(err));
-            error_free(err);
-        }
         g_assert(creds != NULL);
     }
 
diff --git a/tests/test-crypto-tlssession.c b/tests/test-crypto-tlssession.c
index 7bd811796e..fd9acf9067 100644
--- a/tests/test-crypto-tlssession.c
+++ b/tests/test-crypto-tlssession.c
@@ -52,28 +52,21 @@ static ssize_t testRead(char *buf, size_t len, void *opaque)
 
 static QCryptoTLSCreds *test_tls_creds_psk_create(
     QCryptoTLSCredsEndpoint endpoint,
-    const char *dir,
-    Error **errp)
+    const char *dir)
 {
-    Error *err = NULL;
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(
         TYPE_QCRYPTO_TLS_CREDS_PSK,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        &err,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", dir,
         "priority", "NORMAL",
         NULL
         );
-
-    if (err) {
-        error_propagate(errp, err);
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 
@@ -87,7 +80,6 @@ static void test_crypto_tls_session_psk(void)
     int channel[2];
     bool clientShake = false;
     bool serverShake = false;
-    Error *err = NULL;
     int ret;
 
     /* We'll use this for our fake client-server connection */
@@ -104,25 +96,23 @@ static void test_crypto_tls_session_psk(void)
 
     clientCreds = test_tls_creds_psk_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        WORKDIR,
-        &err);
+        WORKDIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_psk_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        WORKDIR,
-        &err);
+        WORKDIR);
     g_assert(serverCreds != NULL);
 
     /* Now the real part of the test, setup the sessions */
     clientSess = qcrypto_tls_session_new(
         clientCreds, NULL, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
+        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
+    g_assert(clientSess != NULL);
+
     serverSess = qcrypto_tls_session_new(
         serverCreds, NULL, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
-
-    g_assert(clientSess != NULL);
+        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
     g_assert(serverSess != NULL);
 
     /* For handshake to work, we need to set the I/O callbacks
@@ -145,7 +135,7 @@ static void test_crypto_tls_session_psk(void)
         int rv;
         if (!serverShake) {
             rv = qcrypto_tls_session_handshake(serverSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(serverSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
@@ -154,7 +144,7 @@ static void test_crypto_tls_session_psk(void)
         }
         if (!clientShake) {
             rv = qcrypto_tls_session_handshake(clientSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(clientSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
@@ -165,8 +155,10 @@ static void test_crypto_tls_session_psk(void)
 
 
     /* Finally make sure the server & client validation is successful. */
-    g_assert(qcrypto_tls_session_check_credentials(serverSess, &err) == 0);
-    g_assert(qcrypto_tls_session_check_credentials(clientSess, &err) == 0);
+    g_assert(qcrypto_tls_session_check_credentials(serverSess,
+                                                   &error_abort) == 0);
+    g_assert(qcrypto_tls_session_check_credentials(clientSess,
+                                                   &error_abort) == 0);
 
     object_unparent(OBJECT(serverCreds));
     object_unparent(OBJECT(clientCreds));
@@ -192,17 +184,15 @@ struct QCryptoTLSSessionTestData {
 
 static QCryptoTLSCreds *test_tls_creds_x509_create(
     QCryptoTLSCredsEndpoint endpoint,
-    const char *certdir,
-    Error **errp)
+    const char *certdir)
 {
-    Error *err = NULL;
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(
         TYPE_QCRYPTO_TLS_CREDS_X509,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        &err,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", certdir,
@@ -217,11 +207,6 @@ static QCryptoTLSCreds *test_tls_creds_x509_create(
         "sanity-check", "no",
         NULL
         );
-
-    if (err) {
-        error_propagate(errp, err);
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 
@@ -249,7 +234,6 @@ static void test_crypto_tls_session_x509(const void *opaque)
     int channel[2];
     bool clientShake = false;
     bool serverShake = false;
-    Error *err = NULL;
     int ret;
 
     /* We'll use this for our fake client-server connection */
@@ -293,14 +277,12 @@ static void test_crypto_tls_session_x509(const void *opaque)
 
     clientCreds = test_tls_creds_x509_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        CLIENT_CERT_DIR,
-        &err);
+        CLIENT_CERT_DIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_x509_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        SERVER_CERT_DIR,
-        &err);
+        SERVER_CERT_DIR);
     g_assert(serverCreds != NULL);
 
     acl = qemu_acl_init("tlssessionacl");
@@ -314,13 +296,13 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /* Now the real part of the test, setup the sessions */
     clientSess = qcrypto_tls_session_new(
         clientCreds, data->hostname, NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
+        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
+    g_assert(clientSess != NULL);
+
     serverSess = qcrypto_tls_session_new(
         serverCreds, NULL,
         data->wildcards ? "tlssessionacl" : NULL,
-        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
-
-    g_assert(clientSess != NULL);
+        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
     g_assert(serverSess != NULL);
 
     /* For handshake to work, we need to set the I/O callbacks
@@ -343,7 +325,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
         int rv;
         if (!serverShake) {
             rv = qcrypto_tls_session_handshake(serverSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(serverSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
@@ -352,7 +334,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
         }
         if (!clientShake) {
             rv = qcrypto_tls_session_handshake(clientSess,
-                                               &err);
+                                               &error_abort);
             g_assert(rv >= 0);
             if (qcrypto_tls_session_get_handshake_status(clientSess) ==
                 QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
@@ -365,10 +347,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /* Finally make sure the server validation does what
      * we were expecting
      */
-    if (qcrypto_tls_session_check_credentials(serverSess, &err) < 0) {
+    if (qcrypto_tls_session_check_credentials(
+            serverSess, data->expectServerFail ? NULL : &error_abort) < 0) {
         g_assert(data->expectServerFail);
-        error_free(err);
-        err = NULL;
     } else {
         g_assert(!data->expectServerFail);
     }
@@ -376,10 +357,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
     /*
      * And the same for the client validation check
      */
-    if (qcrypto_tls_session_check_credentials(clientSess, &err) < 0) {
+    if (qcrypto_tls_session_check_credentials(
+            clientSess, data->expectClientFail ? NULL : &error_abort) < 0) {
         g_assert(data->expectClientFail);
-        error_free(err);
-        err = NULL;
     } else {
         g_assert(!data->expectClientFail);
     }
diff --git a/tests/test-io-channel-tls.c b/tests/test-io-channel-tls.c
index bb88ee870f..4900c6d433 100644
--- a/tests/test-io-channel-tls.c
+++ b/tests/test-io-channel-tls.c
@@ -30,6 +30,7 @@
 #include "crypto/init.h"
 #include "crypto/tlscredsx509.h"
 #include "qemu/acl.h"
+#include "qapi/error.h"
 #include "qom/object_interfaces.h"
 
 #ifdef QCRYPTO_HAVE_TLS_TEST_SUPPORT
@@ -64,8 +65,7 @@ static void test_tls_handshake_done(QIOTask *task,
 
 
 static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
-                                              const char *certdir,
-                                              Error **errp)
+                                              const char *certdir)
 {
     Object *parent = object_get_objects_root();
     Object *creds = object_new_with_props(
@@ -73,7 +73,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         parent,
         (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
          "testtlscredsserver" : "testtlscredsclient"),
-        errp,
+        &error_abort,
         "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
                      "server" : "client"),
         "dir", certdir,
@@ -89,9 +89,6 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
         NULL
         );
 
-    if (*errp) {
-        return NULL;
-    }
     return QCRYPTO_TLS_CREDS(creds);
 }
 
@@ -121,7 +118,6 @@ static void test_io_channel_tls(const void *opaque)
     int channel[2];
     struct QIOChannelTLSHandshakeData clientHandshake = { false, false };
     struct QIOChannelTLSHandshakeData serverHandshake = { false, false };
-    Error *err = NULL;
     QIOChannelTest *test;
     GMainContext *mainloop;
 
@@ -157,14 +153,12 @@ static void test_io_channel_tls(const void *opaque)
 
     clientCreds = test_tls_creds_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
-        CLIENT_CERT_DIR,
-        &err);
+        CLIENT_CERT_DIR);
     g_assert(clientCreds != NULL);
 
     serverCreds = test_tls_creds_create(
         QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
-        SERVER_CERT_DIR,
-        &err);
+        SERVER_CERT_DIR);
     g_assert(serverCreds != NULL);
 
     acl = qemu_acl_init("channeltlsacl");
@@ -176,10 +170,10 @@ static void test_io_channel_tls(const void *opaque)
     }
 
     clientChanSock = qio_channel_socket_new_fd(
-        channel[0], &err);
+        channel[0], &error_abort);
     g_assert(clientChanSock != NULL);
     serverChanSock = qio_channel_socket_new_fd(
-        channel[1], &err);
+        channel[1], &error_abort);
     g_assert(serverChanSock != NULL);
 
     /*
@@ -193,12 +187,12 @@ static void test_io_channel_tls(const void *opaque)
     /* Now the real part of the test, setup the sessions */
     clientChanTLS = qio_channel_tls_new_client(
         QIO_CHANNEL(clientChanSock), clientCreds,
-        data->hostname, &err);
+        data->hostname, &error_abort);
     g_assert(clientChanTLS != NULL);
 
     serverChanTLS = qio_channel_tls_new_server(
         QIO_CHANNEL(serverChanSock), serverCreds,
-        "channeltlsacl", &err);
+        "channeltlsacl", &error_abort);
     g_assert(serverChanTLS != NULL);
 
     qio_channel_tls_handshake(clientChanTLS,
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.0 3/4] tests: use error_abort in places expecting errors

[Philippe Mathieu-Daudé]

On 07/18/2018 06:38 AM, Daniel P. Berrangé wrote:
> Most of the TLS related tests are passing an in a "Error" object to
> methods that are expected to fail, but then ignoring any error that is
> set and instead asserting on a return value. This means that when an
> error is unexpectedly raised, no information about it is printed out,
> making failures hard to diagnose. Changing these tests to pass in
> &error_abort will make unexpected failures print messages to stderr.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/test-crypto-tlscredsx509.c | 11 +----
>  tests/test-crypto-tlssession.c   | 76 ++++++++++++--------------------
>  tests/test-io-channel-tls.c      | 24 ++++------
>  3 files changed, 39 insertions(+), 72 deletions(-)
> 
> diff --git a/tests/test-crypto-tlscredsx509.c b/tests/test-crypto-tlscredsx509.c
> index af2f80e89c..30f9ac4bbf 100644
> --- a/tests/test-crypto-tlscredsx509.c
> +++ b/tests/test-crypto-tlscredsx509.c
> @@ -54,7 +54,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
>          "sanity-check", "yes",
>          NULL);
>  
> -    if (*errp) {
> +    if (!creds) {
>          return NULL;
>      }
>      return QCRYPTO_TLS_CREDS(creds);
> @@ -74,7 +74,6 @@ static void test_tls_creds(const void *opaque)
>      struct QCryptoTLSCredsTestData *data =
>          (struct QCryptoTLSCredsTestData *)opaque;
>      QCryptoTLSCreds *creds;
> -    Error *err = NULL;
>  
>  #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/"
>      mkdir(CERT_DIR, 0700);
> @@ -113,17 +112,11 @@ static void test_tls_creds(const void *opaque)
>           QCRYPTO_TLS_CREDS_ENDPOINT_SERVER :
>           QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT),
>          CERT_DIR,
> -        &err);
> +        data->expectFail ? NULL : &error_abort);
>  
>      if (data->expectFail) {
> -        error_free(err);
>          g_assert(creds == NULL);
>      } else {
> -        if (err) {
> -            g_printerr("Failed to generate creds: %s\n",
> -                       error_get_pretty(err));
> -            error_free(err);
> -        }
>          g_assert(creds != NULL);
>      }
>  
> diff --git a/tests/test-crypto-tlssession.c b/tests/test-crypto-tlssession.c
> index 7bd811796e..fd9acf9067 100644
> --- a/tests/test-crypto-tlssession.c
> +++ b/tests/test-crypto-tlssession.c
> @@ -52,28 +52,21 @@ static ssize_t testRead(char *buf, size_t len, void *opaque)
>  
>  static QCryptoTLSCreds *test_tls_creds_psk_create(
>      QCryptoTLSCredsEndpoint endpoint,
> -    const char *dir,
> -    Error **errp)
> +    const char *dir)
>  {
> -    Error *err = NULL;
>      Object *parent = object_get_objects_root();
>      Object *creds = object_new_with_props(
>          TYPE_QCRYPTO_TLS_CREDS_PSK,
>          parent,
>          (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>           "testtlscredsserver" : "testtlscredsclient"),
> -        &err,
> +        &error_abort,
>          "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>                       "server" : "client"),
>          "dir", dir,
>          "priority", "NORMAL",
>          NULL
>          );
> -
> -    if (err) {
> -        error_propagate(errp, err);
> -        return NULL;
> -    }
>      return QCRYPTO_TLS_CREDS(creds);
>  }
>  
> @@ -87,7 +80,6 @@ static void test_crypto_tls_session_psk(void)
>      int channel[2];
>      bool clientShake = false;
>      bool serverShake = false;
> -    Error *err = NULL;
>      int ret;
>  
>      /* We'll use this for our fake client-server connection */
> @@ -104,25 +96,23 @@ static void test_crypto_tls_session_psk(void)
>  
>      clientCreds = test_tls_creds_psk_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
> -        WORKDIR,
> -        &err);
> +        WORKDIR);
>      g_assert(clientCreds != NULL);
>  
>      serverCreds = test_tls_creds_psk_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
> -        WORKDIR,
> -        &err);
> +        WORKDIR);
>      g_assert(serverCreds != NULL);
>  
>      /* Now the real part of the test, setup the sessions */
>      clientSess = qcrypto_tls_session_new(
>          clientCreds, NULL, NULL,
> -        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
> +        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
> +    g_assert(clientSess != NULL);
> +
>      serverSess = qcrypto_tls_session_new(
>          serverCreds, NULL, NULL,
> -        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
> -
> -    g_assert(clientSess != NULL);
> +        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
>      g_assert(serverSess != NULL);
>  
>      /* For handshake to work, we need to set the I/O callbacks
> @@ -145,7 +135,7 @@ static void test_crypto_tls_session_psk(void)
>          int rv;
>          if (!serverShake) {
>              rv = qcrypto_tls_session_handshake(serverSess,
> -                                               &err);
> +                                               &error_abort);
>              g_assert(rv >= 0);
>              if (qcrypto_tls_session_get_handshake_status(serverSess) ==
>                  QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
> @@ -154,7 +144,7 @@ static void test_crypto_tls_session_psk(void)
>          }
>          if (!clientShake) {
>              rv = qcrypto_tls_session_handshake(clientSess,
> -                                               &err);
> +                                               &error_abort);
>              g_assert(rv >= 0);
>              if (qcrypto_tls_session_get_handshake_status(clientSess) ==
>                  QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
> @@ -165,8 +155,10 @@ static void test_crypto_tls_session_psk(void)
>  
>  
>      /* Finally make sure the server & client validation is successful. */
> -    g_assert(qcrypto_tls_session_check_credentials(serverSess, &err) == 0);
> -    g_assert(qcrypto_tls_session_check_credentials(clientSess, &err) == 0);
> +    g_assert(qcrypto_tls_session_check_credentials(serverSess,
> +                                                   &error_abort) == 0);
> +    g_assert(qcrypto_tls_session_check_credentials(clientSess,
> +                                                   &error_abort) == 0);
>  
>      object_unparent(OBJECT(serverCreds));
>      object_unparent(OBJECT(clientCreds));
> @@ -192,17 +184,15 @@ struct QCryptoTLSSessionTestData {
>  
>  static QCryptoTLSCreds *test_tls_creds_x509_create(
>      QCryptoTLSCredsEndpoint endpoint,
> -    const char *certdir,
> -    Error **errp)
> +    const char *certdir)
>  {
> -    Error *err = NULL;
>      Object *parent = object_get_objects_root();
>      Object *creds = object_new_with_props(
>          TYPE_QCRYPTO_TLS_CREDS_X509,
>          parent,
>          (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>           "testtlscredsserver" : "testtlscredsclient"),
> -        &err,
> +        &error_abort,
>          "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>                       "server" : "client"),
>          "dir", certdir,
> @@ -217,11 +207,6 @@ static QCryptoTLSCreds *test_tls_creds_x509_create(
>          "sanity-check", "no",
>          NULL
>          );
> -
> -    if (err) {
> -        error_propagate(errp, err);
> -        return NULL;
> -    }
>      return QCRYPTO_TLS_CREDS(creds);
>  }
>  
> @@ -249,7 +234,6 @@ static void test_crypto_tls_session_x509(const void *opaque)
>      int channel[2];
>      bool clientShake = false;
>      bool serverShake = false;
> -    Error *err = NULL;
>      int ret;
>  
>      /* We'll use this for our fake client-server connection */
> @@ -293,14 +277,12 @@ static void test_crypto_tls_session_x509(const void *opaque)
>  
>      clientCreds = test_tls_creds_x509_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
> -        CLIENT_CERT_DIR,
> -        &err);
> +        CLIENT_CERT_DIR);
>      g_assert(clientCreds != NULL);
>  
>      serverCreds = test_tls_creds_x509_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
> -        SERVER_CERT_DIR,
> -        &err);
> +        SERVER_CERT_DIR);
>      g_assert(serverCreds != NULL);
>  
>      acl = qemu_acl_init("tlssessionacl");
> @@ -314,13 +296,13 @@ static void test_crypto_tls_session_x509(const void *opaque)
>      /* Now the real part of the test, setup the sessions */
>      clientSess = qcrypto_tls_session_new(
>          clientCreds, data->hostname, NULL,
> -        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &err);
> +        QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, &error_abort);
> +    g_assert(clientSess != NULL);
> +
>      serverSess = qcrypto_tls_session_new(
>          serverCreds, NULL,
>          data->wildcards ? "tlssessionacl" : NULL,
> -        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &err);
> -
> -    g_assert(clientSess != NULL);
> +        QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, &error_abort);
>      g_assert(serverSess != NULL);
>  
>      /* For handshake to work, we need to set the I/O callbacks
> @@ -343,7 +325,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
>          int rv;
>          if (!serverShake) {
>              rv = qcrypto_tls_session_handshake(serverSess,
> -                                               &err);
> +                                               &error_abort);
>              g_assert(rv >= 0);
>              if (qcrypto_tls_session_get_handshake_status(serverSess) ==
>                  QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
> @@ -352,7 +334,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
>          }
>          if (!clientShake) {
>              rv = qcrypto_tls_session_handshake(clientSess,
> -                                               &err);
> +                                               &error_abort);
>              g_assert(rv >= 0);
>              if (qcrypto_tls_session_get_handshake_status(clientSess) ==
>                  QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
> @@ -365,10 +347,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
>      /* Finally make sure the server validation does what
>       * we were expecting
>       */
> -    if (qcrypto_tls_session_check_credentials(serverSess, &err) < 0) {
> +    if (qcrypto_tls_session_check_credentials(
> +            serverSess, data->expectServerFail ? NULL : &error_abort) < 0) {
>          g_assert(data->expectServerFail);
> -        error_free(err);
> -        err = NULL;
>      } else {
>          g_assert(!data->expectServerFail);
>      }
> @@ -376,10 +357,9 @@ static void test_crypto_tls_session_x509(const void *opaque)
>      /*
>       * And the same for the client validation check
>       */
> -    if (qcrypto_tls_session_check_credentials(clientSess, &err) < 0) {
> +    if (qcrypto_tls_session_check_credentials(
> +            clientSess, data->expectClientFail ? NULL : &error_abort) < 0) {
>          g_assert(data->expectClientFail);
> -        error_free(err);
> -        err = NULL;
>      } else {
>          g_assert(!data->expectClientFail);
>      }
> diff --git a/tests/test-io-channel-tls.c b/tests/test-io-channel-tls.c
> index bb88ee870f..4900c6d433 100644
> --- a/tests/test-io-channel-tls.c
> +++ b/tests/test-io-channel-tls.c
> @@ -30,6 +30,7 @@
>  #include "crypto/init.h"
>  #include "crypto/tlscredsx509.h"
>  #include "qemu/acl.h"
> +#include "qapi/error.h"
>  #include "qom/object_interfaces.h"
>  
>  #ifdef QCRYPTO_HAVE_TLS_TEST_SUPPORT
> @@ -64,8 +65,7 @@ static void test_tls_handshake_done(QIOTask *task,
>  
>  
>  static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
> -                                              const char *certdir,
> -                                              Error **errp)
> +                                              const char *certdir)
>  {
>      Object *parent = object_get_objects_root();
>      Object *creds = object_new_with_props(
> @@ -73,7 +73,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
>          parent,
>          (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>           "testtlscredsserver" : "testtlscredsclient"),
> -        errp,
> +        &error_abort,
>          "endpoint", (endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER ?
>                       "server" : "client"),
>          "dir", certdir,
> @@ -89,9 +89,6 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint,
>          NULL
>          );
>  
> -    if (*errp) {
> -        return NULL;
> -    }
>      return QCRYPTO_TLS_CREDS(creds);
>  }
>  
> @@ -121,7 +118,6 @@ static void test_io_channel_tls(const void *opaque)
>      int channel[2];
>      struct QIOChannelTLSHandshakeData clientHandshake = { false, false };
>      struct QIOChannelTLSHandshakeData serverHandshake = { false, false };
> -    Error *err = NULL;
>      QIOChannelTest *test;
>      GMainContext *mainloop;
>  
> @@ -157,14 +153,12 @@ static void test_io_channel_tls(const void *opaque)
>  
>      clientCreds = test_tls_creds_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
> -        CLIENT_CERT_DIR,
> -        &err);
> +        CLIENT_CERT_DIR);
>      g_assert(clientCreds != NULL);
>  
>      serverCreds = test_tls_creds_create(
>          QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
> -        SERVER_CERT_DIR,
> -        &err);
> +        SERVER_CERT_DIR);
>      g_assert(serverCreds != NULL);
>  
>      acl = qemu_acl_init("channeltlsacl");
> @@ -176,10 +170,10 @@ static void test_io_channel_tls(const void *opaque)
>      }
>  
>      clientChanSock = qio_channel_socket_new_fd(
> -        channel[0], &err);
> +        channel[0], &error_abort);
>      g_assert(clientChanSock != NULL);
>      serverChanSock = qio_channel_socket_new_fd(
> -        channel[1], &err);
> +        channel[1], &error_abort);
>      g_assert(serverChanSock != NULL);
>  
>      /*
> @@ -193,12 +187,12 @@ static void test_io_channel_tls(const void *opaque)
>      /* Now the real part of the test, setup the sessions */
>      clientChanTLS = qio_channel_tls_new_client(
>          QIO_CHANNEL(clientChanSock), clientCreds,
> -        data->hostname, &err);
> +        data->hostname, &error_abort);
>      g_assert(clientChanTLS != NULL);
>  
>      serverChanTLS = qio_channel_tls_new_server(
>          QIO_CHANNEL(serverChanSock), serverCreds,
> -        "channeltlsacl", &err);
> +        "channeltlsacl", &error_abort);
>      g_assert(serverChanTLS != NULL);
>  
>      qio_channel_tls_handshake(clientChanTLS,
> 

[Qemu-devel] [PATCH for 3.0 4/4] tests: fix TLS handshake failure with TLS 1.3

[Daniel P. Berrangé]

When gnutls negotiates TLS 1.3 instead of 1.2, the order of messages
sent by the handshake changes. This exposed a logic bug in the test
suite which caused us to wait for the server to see handshake
completion, but not wait for the client to see completion. The result
was the client didn't receive the certificate for verification and the
test failed.

This is exposed in Fedora 29 rawhide which has just enabled TLS 1.3 in
its GNUTLS builds.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/test-crypto-tlssession.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-crypto-tlssession.c b/tests/test-crypto-tlssession.c
index fd9acf9067..6fa9950afb 100644
--- a/tests/test-crypto-tlssession.c
+++ b/tests/test-crypto-tlssession.c
@@ -151,7 +151,7 @@ static void test_crypto_tls_session_psk(void)
                 clientShake = true;
             }
         }
-    } while (!clientShake && !serverShake);
+    } while (!clientShake || !serverShake);
 
 
     /* Finally make sure the server & client validation is successful. */
@@ -341,7 +341,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
                 clientShake = true;
             }
         }
-    } while (!clientShake && !serverShake);
+    } while (!clientShake || !serverShake);
 
 
     /* Finally make sure the server validation does what
-- 
2.17.1

Re: [Qemu-devel] [PATCH for 3.0 4/4] tests: fix TLS handshake failure with TLS 1.3

[Eric Blake]

On 07/18/2018 04:38 AM, Daniel P. Berrangé wrote:
> When gnutls negotiates TLS 1.3 instead of 1.2, the order of messages
> sent by the handshake changes. This exposed a logic bug in the test
> suite which caused us to wait for the server to see handshake
> completion, but not wait for the client to see completion. The result
> was the client didn't receive the certificate for verification and the
> test failed.
> 
> This is exposed in Fedora 29 rawhide which has just enabled TLS 1.3 in
> its GNUTLS builds.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/test-crypto-tlssession.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org