From a6dd675fd3e9269a4899d9a58def1896efea9fe2 Mon Sep 17 00:00:00 2001 From: david Date: Fri, 4 Jan 2013 18:59:56 +0000 Subject: [PATCH] Don't do operations with side effects inside asserts. ncat_assert is safe because it cannot be disabled. assert is also safe because we make sure that NDEBUG remains defined. Doing this helps avoid potential bad effects of something changing in the future. --- ncat/ncat_connect.c | 8 +++++-- ncat/ncat_exec_win.c | 52 +++++++++++++++++++++++++++++--------------- ncat/ncat_posix.c | 3 ++- ncat/ncat_ssl.c | 4 +++- 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/ncat/ncat_connect.c b/ncat/ncat_connect.c index 8d6e1a8f8..7cf21d6f7 100644 --- a/ncat/ncat_connect.c +++ b/ncat/ncat_connect.c @@ -155,6 +155,7 @@ static int verify_callback(int ok, X509_STORE_CTX *store) level. */ if ((!ok && o.verbose) || o.debug > 1) { char digest_buf[SHA1_STRING_LENGTH + 1]; + char *fp; loguser("Subject: "); X509_NAME_print_ex_fp(stderr, X509_get_subject_name(cert), 0, XN_FLAG_COMPAT); @@ -163,7 +164,8 @@ static int verify_callback(int ok, X509_STORE_CTX *store) X509_NAME_print_ex_fp(stderr, X509_get_issuer_name(cert), 0, XN_FLAG_COMPAT); loguser_noprefix("\n"); - ncat_assert(ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)) != NULL); + fp = ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)); + ncat_assert(fp == digest_buf); loguser("SHA-1 fingerprint: %s\n", digest_buf); } @@ -222,6 +224,7 @@ static void connect_report(nsock_iod nsi) X509 *cert; X509_NAME *subject; char digest_buf[SHA1_STRING_LENGTH + 1]; + char *fp; loguser("SSL connection to %s:%hu.", inet_socktop(&peer), nsi_peerport(nsi)); @@ -240,7 +243,8 @@ static void connect_report(nsock_iod nsi) loguser_noprefix("\n"); - ncat_assert(ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)) != NULL); + fp = ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)); + ncat_assert(fp == digest_buf); loguser("SHA-1 fingerprint: %s\n", digest_buf); } else { #if HAVE_SYS_UN_H diff --git a/ncat/ncat_exec_win.c b/ncat/ncat_exec_win.c index 6efdc3269..3540761ba 100644 --- a/ncat/ncat_exec_win.c +++ b/ncat/ncat_exec_win.c @@ -182,13 +182,17 @@ void netexec(struct fdinfo *fdn, char *cmdexec) child process dies. This is only used on Windows. */ extern void set_pseudo_sigchld_handler(void (*handler)(void)) { + DWORD rc; + if (pseudo_sigchld_mutex == NULL) { pseudo_sigchld_mutex = CreateMutex(NULL, FALSE, NULL); ncat_assert(pseudo_sigchld_mutex != NULL); } - ncat_assert(WaitForSingleObject(pseudo_sigchld_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(pseudo_sigchld_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); pseudo_sigchld_handler = handler; - ncat_assert(ReleaseMutex(pseudo_sigchld_mutex) != 0); + rc = ReleaseMutex(pseudo_sigchld_mutex); + ncat_assert(rc != 0); } /* Run a command and redirect its input and output handles to a pair of @@ -365,7 +369,7 @@ static DWORD WINAPI subprocess_thread_func(void *data) char pipe_buffer[BUFSIZ]; OVERLAPPED overlap = { 0 }; HANDLE events[3]; - DWORD ret; + DWORD ret, rc; int crlf_state = 0; info = (struct subprocess_info *) data; @@ -448,12 +452,11 @@ loop_end: WSACloseEvent(events[0]); - ncat_assert(unregister_subprocess(info->proc) != -1); + rc = unregister_subprocess(info->proc); + ncat_assert(rc != -1); GetExitCodeProcess(info->proc, &ret); if (ret == STILL_ACTIVE) { - DWORD rc; - if (o.debug > 1) logdebug("Subprocess still running, terminating it.\n"); rc = TerminateProcess(info->proc, 0); @@ -470,10 +473,12 @@ loop_end: subprocess_info_close(info); free(info); - ncat_assert(WaitForSingleObject(pseudo_sigchld_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(pseudo_sigchld_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); if (pseudo_sigchld_handler != NULL) pseudo_sigchld_handler(); - ncat_assert(ReleaseMutex(pseudo_sigchld_mutex) != 0); + rc = ReleaseMutex(pseudo_sigchld_mutex); + ncat_assert(rc != 0); return ret; } @@ -485,14 +490,15 @@ loop_end: static int get_subprocess_slot(void) { int i, free_index, max_index; + DWORD rc; - ncat_assert(WaitForSingleObject(subprocesses_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(subprocesses_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); free_index = -1; max_index = 0; for (i = 0; i < subprocess_max_index; i++) { HANDLE proc = subprocesses[i]; - DWORD ret; if (proc == NULL) { if (free_index == -1) @@ -506,7 +512,8 @@ static int get_subprocess_slot(void) free_index = max_index++; subprocess_max_index = max_index; - ncat_assert(ReleaseMutex(subprocesses_mutex) != 0); + rc = ReleaseMutex(subprocesses_mutex); + ncat_assert(rc != 0); return free_index; } @@ -517,7 +524,8 @@ static int get_subprocess_slot(void) error. */ static int register_subprocess(HANDLE proc) { - int i, rc; + int i; + DWORD rc; if (subprocesses_mutex == NULL) { subprocesses_mutex = CreateMutex(NULL, FALSE, NULL); @@ -528,7 +536,8 @@ static int register_subprocess(HANDLE proc) ncat_assert(pseudo_sigchld_mutex != NULL); } - ncat_assert(WaitForSingleObject(subprocesses_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(subprocesses_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); i = get_subprocess_slot(); if (i == -1) { @@ -549,7 +558,8 @@ static int register_subprocess(HANDLE proc) } } - ncat_assert(ReleaseMutex(subprocesses_mutex) != 0); + rc = ReleaseMutex(subprocesses_mutex); + ncat_assert(rc != 0); return i; } @@ -559,8 +569,10 @@ static int register_subprocess(HANDLE proc) static int unregister_subprocess(HANDLE proc) { int i; + DWORD rc; - ncat_assert(WaitForSingleObject(subprocesses_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(subprocesses_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); for (i = 0; i < subprocess_max_index; i++) { if (proc == subprocesses[i]) @@ -574,7 +586,8 @@ static int unregister_subprocess(HANDLE proc) i = -1; } - ncat_assert(ReleaseMutex(subprocesses_mutex) != 0); + rc = ReleaseMutex(subprocesses_mutex); + ncat_assert(rc != 0); return i; } @@ -582,11 +595,13 @@ static int unregister_subprocess(HANDLE proc) static void terminate_subprocesses(void) { int i; + DWORD rc; if (o.debug) logdebug("Terminating subprocesses\n"); - ncat_assert(WaitForSingleObject(subprocesses_mutex, INFINITE) == WAIT_OBJECT_0); + rc = WaitForSingleObject(subprocesses_mutex, INFINITE); + ncat_assert(rc == WAIT_OBJECT_0); if (o.debug > 1) logdebug("max_index %d\n", subprocess_max_index); @@ -605,7 +620,8 @@ static void terminate_subprocesses(void) subprocesses[i] = NULL; } - ncat_assert(ReleaseMutex(subprocesses_mutex) != 0); + rc = ReleaseMutex(subprocesses_mutex); + ncat_assert(rc != 0); } static void sigint_handler(int s) diff --git a/ncat/ncat_posix.c b/ncat/ncat_posix.c index afac3f333..fab397427 100644 --- a/ncat/ncat_posix.c +++ b/ncat/ncat_posix.c @@ -348,7 +348,8 @@ int ssl_load_default_ca_certs(SSL_CTX *ctx) logdebug("Using system default trusted CA certificates and those in %s.\n", NCAT_CA_CERTS_PATH); /* Load distribution-provided defaults, if any. */ - ncat_assert(SSL_CTX_set_default_verify_paths(ctx) > 0); + rc = SSL_CTX_set_default_verify_paths(ctx); + ncat_assert(rc > 0); /* Also load the trusted certificates we ship. */ rc = SSL_CTX_load_verify_locations(ctx, NCAT_CA_CERTS_PATH, NULL); diff --git a/ncat/ncat_ssl.c b/ncat/ncat_ssl.c index d7a94b5f5..518dc3ba6 100644 --- a/ncat/ncat_ssl.c +++ b/ncat/ncat_ssl.c @@ -159,7 +159,9 @@ SSL_CTX *setup_ssl_listen(void) if (ssl_gen_cert(&cert, &key) == 0) bye("ssl_gen_cert(): %s.", ERR_error_string(ERR_get_error(), NULL)); if (o.verbose) { - ncat_assert(ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)) != NULL); + char *fp; + fp = ssl_cert_fp_str_sha1(cert, digest_buf, sizeof(digest_buf)); + ncat_assert(fp == digest_buf); loguser("SHA-1 fingerprint: %s\n", digest_buf); } if (SSL_CTX_use_certificate(sslctx, cert) != 1)