Formatting and style guide improvements for opensslstreamadapter.cc
This change is part of a long set of changes to improve the overall code quality
of the the cryptography code in WebRTC. This is a set of low risk refactorings.
More complex refactorings will be saved for a different CL.
This change updates the conditions to move away from:
if (a)
b = c;
to
if (a) {
b = c;
}
The code style guide allows for either but in security critical code this has
been known to cause issues as it is very easy to forget the braces when
adding additional code to conditionals.
Bug: webrtc:9860
Change-Id: I2ec07a4129fe4756b90f6b295d62a4cadbc1f71f
Reviewed-on: https://webrtc-review.googlesource.com/c/106140
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Commit-Queue: Benjamin Wright <benwright@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25186}diff --git a/rtc_base/opensslstreamadapter.cc b/rtc_base/opensslstreamadapter.cc
index fd54a08..32a2752 100644
--- a/rtc_base/opensslstreamadapter.cc
+++ b/rtc_base/opensslstreamadapter.cc
@@ -22,6 +22,7 @@
#endif
#include <memory>
+#include <utility>
#include <vector>
#include "rtc_base/checks.h"
@@ -180,8 +181,9 @@
static BIO* BIO_new_stream(StreamInterface* stream) {
BIO* ret = BIO_new(BIO_stream_method());
- if (ret == nullptr)
+ if (ret == nullptr) {
return nullptr;
+ }
BIO_set_data(ret, stream);
return ret;
}
@@ -196,14 +198,16 @@
}
static int stream_free(BIO* b) {
- if (b == nullptr)
+ if (b == nullptr) {
return 0;
+ }
return 1;
}
static int stream_read(BIO* b, char* out, int outl) {
- if (!out)
+ if (!out) {
return -1;
+ }
StreamInterface* stream = static_cast<StreamInterface*>(BIO_get_data(b));
BIO_clear_retry_flags(b);
size_t read;
@@ -218,8 +222,9 @@
}
static int stream_write(BIO* b, const char* in, int inl) {
- if (!in)
+ if (!in) {
return -1;
+ }
StreamInterface* stream = static_cast<StreamInterface*>(BIO_get_data(b));
BIO_clear_retry_flags(b);
size_t written;
@@ -362,8 +367,9 @@
}
bool OpenSSLStreamAdapter::GetSslCipherSuite(int* cipher_suite) {
- if (state_ != SSL_CONNECTED)
+ if (state_ != SSL_CONNECTED) {
return false;
+ }
const SSL_CIPHER* current_cipher = SSL_get_current_cipher(ssl_);
if (current_cipher == nullptr) {
@@ -380,17 +386,19 @@
int ssl_version = SSL_version(ssl_);
if (ssl_mode_ == SSL_MODE_DTLS) {
- if (ssl_version == DTLS1_VERSION)
+ if (ssl_version == DTLS1_VERSION) {
return SSL_PROTOCOL_DTLS_10;
- else if (ssl_version == DTLS1_2_VERSION)
+ } else if (ssl_version == DTLS1_2_VERSION) {
return SSL_PROTOCOL_DTLS_12;
+ }
} else {
- if (ssl_version == TLS1_VERSION)
+ if (ssl_version == TLS1_VERSION) {
return SSL_PROTOCOL_TLS_10;
- else if (ssl_version == TLS1_1_VERSION)
+ } else if (ssl_version == TLS1_1_VERSION) {
return SSL_PROTOCOL_TLS_11;
- else if (ssl_version == TLS1_2_VERSION)
+ } else if (ssl_version == TLS1_2_VERSION) {
return SSL_PROTOCOL_TLS_12;
+ }
}
return -1;
@@ -403,15 +411,11 @@
bool use_context,
uint8_t* result,
size_t result_len) {
- int i;
-
- i = SSL_export_keying_material(ssl_, result, result_len, label.c_str(),
+ if (SSL_export_keying_material(ssl_, result, result_len, label.c_str(),
label.length(), const_cast<uint8_t*>(context),
- context_len, use_context);
-
- if (i != 1)
+ context_len, use_context) != 1) {
return false;
-
+ }
return true;
}
@@ -419,8 +423,9 @@
const std::vector<int>& ciphers) {
std::string internal_ciphers;
- if (state_ != SSL_NONE)
+ if (state_ != SSL_NONE) {
return false;
+ }
for (std::vector<int>::const_iterator cipher = ciphers.begin();
cipher != ciphers.end(); ++cipher) {
@@ -429,8 +434,9 @@
++entry) {
if (*cipher == entry->id) {
found = true;
- if (!internal_ciphers.empty())
+ if (!internal_ciphers.empty()) {
internal_ciphers += ":";
+ }
internal_ciphers += entry->internal_name;
break;
}
@@ -442,8 +448,9 @@
}
}
- if (internal_ciphers.empty())
+ if (internal_ciphers.empty()) {
return false;
+ }
srtp_ciphers_ = internal_ciphers;
return true;
@@ -451,14 +458,16 @@
bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) {
RTC_DCHECK(state_ == SSL_CONNECTED);
- if (state_ != SSL_CONNECTED)
+ if (state_ != SSL_CONNECTED) {
return false;
+ }
const SRTP_PROTECTION_PROFILE* srtp_profile =
SSL_get_selected_srtp_profile(ssl_);
- if (!srtp_profile)
+ if (!srtp_profile) {
return false;
+ }
*crypto_suite = srtp_profile->id;
RTC_DCHECK(!SrtpCryptoSuiteToName(*crypto_suite).empty());
@@ -470,8 +479,8 @@
}
int OpenSSLStreamAdapter::StartSSL() {
+ // Don't allow StartSSL to be called twice.
if (state_ != SSL_NONE) {
- // Don't allow StartSSL to be called twice.
return -1;
}
@@ -532,15 +541,17 @@
case SSL_ERROR:
case SSL_CLOSED:
default:
- if (error)
+ if (error) {
*error = ssl_error_code_;
+ }
return SR_ERROR;
}
// OpenSSL will return an error if we try to write zero bytes
if (data_len == 0) {
- if (written)
+ if (written) {
*written = 0;
+ }
return SR_SUCCESS;
}
@@ -567,8 +578,9 @@
case SSL_ERROR_ZERO_RETURN:
default:
Error("SSL_write", (ssl_error ? ssl_error : -1), 0, false);
- if (error)
+ if (error) {
*error = ssl_error_code_;
+ }
return SR_ERROR;
}
// not reached
@@ -583,31 +595,29 @@
case SSL_NONE:
// pass-through in clear text
return StreamAdapterInterface::Read(data, data_len, read, error);
-
case SSL_WAIT:
case SSL_CONNECTING:
return SR_BLOCK;
-
case SSL_CONNECTED:
if (waiting_to_verify_peer_certificate()) {
return SR_BLOCK;
}
break;
-
case SSL_CLOSED:
return SR_EOS;
-
case SSL_ERROR:
default:
- if (error)
+ if (error) {
*error = ssl_error_code_;
+ }
return SR_ERROR;
}
// Don't trust OpenSSL with zero byte reads
if (data_len == 0) {
- if (read)
+ if (read) {
*read = 0;
+ }
return SR_SUCCESS;
}
@@ -620,8 +630,9 @@
RTC_LOG(LS_VERBOSE) << " -- success";
RTC_DCHECK_GT(code, 0);
RTC_DCHECK_LE(code, data_len);
- if (read)
+ if (read) {
*read = code;
+ }
if (ssl_mode_ == SSL_MODE_DTLS) {
// Enforce atomic reads -- this is a short read
@@ -630,8 +641,9 @@
if (pending) {
RTC_LOG(LS_INFO) << " -- short DTLS read. flushing";
FlushInput(pending);
- if (error)
+ if (error) {
*error = SSE_MSG_TRUNC;
+ }
return SR_ERROR;
}
}
@@ -650,8 +662,9 @@
break;
default:
Error("SSL_read", (ssl_error ? ssl_error : -1), 0, false);
- if (error)
+ if (error) {
*error = ssl_error_code_;
+ }
return SR_ERROR;
}
// not reached
@@ -700,7 +713,7 @@
return SS_OPEN;
default:
return SS_CLOSED;
- };
+ }
// not reached
}
@@ -710,6 +723,7 @@
int events_to_signal = 0;
int signal_error = 0;
RTC_DCHECK(stream == this->stream());
+
if ((events & SE_OPEN)) {
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent SE_OPEN";
if (state_ != SSL_WAIT) {
@@ -723,6 +737,7 @@
}
}
}
+
if ((events & (SE_READ | SE_WRITE))) {
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent"
<< ((events & SE_READ) ? " SE_READ" : "")
@@ -747,6 +762,7 @@
}
}
}
+
if ((events & SE_CLOSE)) {
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent(SE_CLOSE, " << err
<< ")";
@@ -756,8 +772,10 @@
RTC_DCHECK(signal_error == 0);
signal_error = err;
}
- if (events_to_signal)
+
+ if (events_to_signal) {
StreamAdapterInterface::OnEvent(stream, events_to_signal, signal_error);
+ }
}
int OpenSSLStreamAdapter::BeginSSL() {
@@ -770,12 +788,14 @@
// First set up the context.
RTC_DCHECK(ssl_ctx_ == nullptr);
ssl_ctx_ = SetupSSLContext();
- if (!ssl_ctx_)
+ if (!ssl_ctx_) {
return -1;
+ }
bio = BIO_new_stream(static_cast<StreamInterface*>(stream()));
- if (!bio)
+ if (!bio) {
return -1;
+ }
ssl_ = SSL_new(ssl_ctx_);
if (!ssl_) {
@@ -805,8 +825,9 @@
// commonly supported. BoringSSL doesn't need explicit configuration and has
// a reasonable default set.
EC_KEY* ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
- if (ecdh == nullptr)
+ if (ecdh == nullptr) {
return -1;
+ }
SSL_set_options(ssl_, SSL_OP_SINGLE_ECDH_USE);
SSL_set_tmp_ecdh(ssl_, ecdh);
EC_KEY_free(ecdh);
@@ -886,8 +907,9 @@
state_ = SSL_ERROR;
ssl_error_code_ = err;
Cleanup(alert);
- if (signal)
+ if (signal) {
StreamAdapterInterface::OnEvent(stream(), SE_CLOSE, err);
+ }
}
void OpenSSLStreamAdapter::Cleanup(uint8_t alert) {
@@ -990,8 +1012,9 @@
ctx = SSL_CTX_new(method);
#endif // OPENSSL_IS_BORINGSSL
- if (ctx == nullptr)
+ if (ctx == nullptr) {
return nullptr;
+ }
#ifdef OPENSSL_IS_BORINGSSL
SSL_CTX_set_min_proto_version(
@@ -1176,15 +1199,17 @@
bool OpenSSLStreamAdapter::IsAcceptableCipher(int cipher, KeyType key_type) {
if (key_type == KT_RSA) {
for (const cipher_list& c : OK_RSA_ciphers) {
- if (cipher == c.cipher)
+ if (cipher == c.cipher) {
return true;
+ }
}
}
if (key_type == KT_ECDSA) {
for (const cipher_list& c : OK_ECDSA_ciphers) {
- if (cipher == c.cipher)
+ if (cipher == c.cipher) {
return true;
+ }
}
}
@@ -1195,15 +1220,17 @@
KeyType key_type) {
if (key_type == KT_RSA) {
for (const cipher_list& c : OK_RSA_ciphers) {
- if (cipher == c.cipher_str)
+ if (cipher == c.cipher_str) {
return true;
+ }
}
}
if (key_type == KT_ECDSA) {
for (const cipher_list& c : OK_ECDSA_ciphers) {
- if (cipher == c.cipher_str)
+ if (cipher == c.cipher_str) {
return true;
+ }
}
}
