From 11840ce684a2411a56a166c8e97a86b217436523 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 10 Nov 2022 10:50:50 +0000 Subject: [PATCH] Deprecate void* forms of StreamInterface::Read and ::Write Updates the code to use the new interfaces Bug: webrtc:14632 Change-Id: I33b2a25b5968de0251e3cbc84076afc013ecef6e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/282680 Reviewed-by: Tomas Gunnarsson Reviewed-by: Danil Chapovalov Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#38601} --- p2p/base/dtls_transport.cc | 15 +++-- rtc_base/openssl_stream_adapter.cc | 90 ++++++++++++++----------- rtc_base/openssl_stream_adapter.h | 21 +++--- rtc_base/ssl_adapter_unittest.cc | 15 +++-- rtc_base/ssl_stream_adapter_unittest.cc | 81 ++++++++++++---------- rtc_base/stream.cc | 7 +- rtc_base/stream.h | 55 +++++++++++---- 7 files changed, 175 insertions(+), 109 deletions(-) diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index 904a0cbbc9..7d9c0b5cce 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -16,6 +16,7 @@ #include "absl/memory/memory.h" #include "absl/strings/string_view.h" +#include "api/array_view.h" #include "api/dtls_transport_interface.h" #include "api/rtc_event_log/rtc_event_log.h" #include "logging/rtc_event_log/events/rtc_event_dtls_transport_state.h" @@ -445,7 +446,12 @@ int DtlsTransport::SendPacket(const char* data, return ice_transport_->SendPacket(data, size, options); } else { - return (dtls_->WriteAll(data, size, NULL, NULL) == rtc::SR_SUCCESS) + size_t written; + int error; + return (dtls_->WriteAll( + rtc::MakeArrayView(reinterpret_cast(data), + size), + written, error) == rtc::SR_SUCCESS) ? static_cast(size) : -1; } @@ -691,16 +697,17 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { } } if (sig & rtc::SE_READ) { - char buf[kMaxDtlsPacketLen]; + uint8_t buf[kMaxDtlsPacketLen]; size_t read; int read_error; rtc::StreamResult ret; // The underlying DTLS stream may have received multiple DTLS records in // one packet, so read all of them. do { - ret = dtls_->Read(buf, sizeof(buf), &read, &read_error); + ret = dtls_->Read(buf, read, read_error); if (ret == rtc::SR_SUCCESS) { - SignalReadPacket(this, buf, read, rtc::TimeMicros(), 0); + SignalReadPacket(this, reinterpret_cast(buf), read, + rtc::TimeMicros(), 0); } else if (ret == rtc::SR_EOS) { // Remote peer shut down the association with no error. RTC_LOG(LS_INFO) << ToString() << ": DTLS transport closed by remote"; diff --git a/rtc_base/openssl_stream_adapter.cc b/rtc_base/openssl_stream_adapter.cc index 61bf6743d6..5fa1a3c502 100644 --- a/rtc_base/openssl_stream_adapter.cc +++ b/rtc_base/openssl_stream_adapter.cc @@ -28,6 +28,7 @@ #include #include +#include "api/array_view.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" @@ -215,7 +216,8 @@ static int stream_read(BIO* b, char* out, int outl) { BIO_clear_retry_flags(b); size_t read; int error; - StreamResult result = stream->Read(out, outl, &read, &error); + StreamResult result = stream->Read( + rtc::MakeArrayView(reinterpret_cast(out), outl), read, error); if (result == SR_SUCCESS) { return checked_cast(read); } else if (result == SR_BLOCK) { @@ -232,7 +234,9 @@ static int stream_write(BIO* b, const char* in, int inl) { BIO_clear_retry_flags(b); size_t written; int error; - StreamResult result = stream->Write(in, inl, &written, &error); + StreamResult result = stream->Write( + rtc::MakeArrayView(reinterpret_cast(in), inl), written, + error); if (result == SR_SUCCESS) { return checked_cast(written); } else if (result == SR_BLOCK) { @@ -557,17 +561,30 @@ void OpenSSLStreamAdapter::SetInitialRetransmissionTimeout(int timeout_ms) { // // StreamInterface Implementation // - +// Backwards compatible Write() method using deprecated API. +// Needed because deprecated API is still =0 in API definition. StreamResult OpenSSLStreamAdapter::Write(const void* data, size_t data_len, size_t* written, int* error) { - RTC_DLOG(LS_VERBOSE) << "OpenSSLStreamAdapter::Write(" << data_len << ")"; + // TODO(bugs.webrtc.org/14632): Consider doing + // RTC_CHECK_NOTREACHED(); when downstream usage is eliminated. + size_t dummy_written; + int dummy_error; + return Write( + rtc::MakeArrayView(reinterpret_cast(data), data_len), + written ? *written : dummy_written, error ? *error : dummy_error); +} + +StreamResult OpenSSLStreamAdapter::Write(rtc::ArrayView data, + size_t& written, + int& error) { + RTC_DLOG(LS_VERBOSE) << "OpenSSLStreamAdapter::Write(" << data.size() << ")"; switch (state_) { case SSL_NONE: // pass-through in clear text - return stream_->Write(data, data_len, written, error); + return stream_->Write(data, written, error); case SSL_WAIT: case SSL_CONNECTING: @@ -582,31 +599,26 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data, case SSL_ERROR: case SSL_CLOSED: default: - if (error) { - *error = ssl_error_code_; - } + 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) { - *written = 0; - } + if (data.size() == 0) { + written = 0; return SR_SUCCESS; } ssl_write_needs_read_ = false; - int code = SSL_write(ssl_, data, checked_cast(data_len)); + int code = SSL_write(ssl_, data.data(), checked_cast(data.size())); int ssl_error = SSL_get_error(ssl_, code); switch (ssl_error) { case SSL_ERROR_NONE: RTC_DLOG(LS_VERBOSE) << " -- success"; RTC_DCHECK_GT(code, 0); - RTC_DCHECK_LE(code, data_len); - if (written) - *written = code; + RTC_DCHECK_LE(code, data.size()); + written = code; return SR_SUCCESS; case SSL_ERROR_WANT_READ: RTC_DLOG(LS_VERBOSE) << " -- error want read"; @@ -619,23 +631,33 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data, case SSL_ERROR_ZERO_RETURN: default: Error("SSL_write", (ssl_error ? ssl_error : -1), 0, false); - if (error) { - *error = ssl_error_code_; - } + error = ssl_error_code_; return SR_ERROR; } // not reached } +// Backwards compatible Read() method using deprecated API. StreamResult OpenSSLStreamAdapter::Read(void* data, size_t data_len, size_t* read, int* error) { - RTC_DLOG(LS_VERBOSE) << "OpenSSLStreamAdapter::Read(" << data_len << ")"; + // TODO(bugs.webrtc.org/14632): Consider doing + // RTC_CHECK_NOTREACHED() when downstream usage is thought to be eliminated. + size_t dummy_read; + int dummy_error; + return Read(rtc::MakeArrayView(reinterpret_cast(data), data_len), + read ? *read : dummy_read, error ? *error : dummy_error); +} + +StreamResult OpenSSLStreamAdapter::Read(rtc::ArrayView data, + size_t& read, + int& error) { + RTC_DLOG(LS_VERBOSE) << "OpenSSLStreamAdapter::Read(" << data.size() << ")"; switch (state_) { case SSL_NONE: // pass-through in clear text - return stream_->Read(data, data_len, read, error); + return stream_->Read(data, read, error); case SSL_WAIT: case SSL_CONNECTING: return SR_BLOCK; @@ -648,33 +670,27 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, return SR_EOS; case SSL_ERROR: default: - if (error) { - *error = ssl_error_code_; - } + error = ssl_error_code_; return SR_ERROR; } // Don't trust OpenSSL with zero byte reads - if (data_len == 0) { - if (read) { - *read = 0; - } + if (data.size() == 0) { + read = 0; return SR_SUCCESS; } ssl_read_needs_write_ = false; - const int code = SSL_read(ssl_, data, checked_cast(data_len)); + const int code = SSL_read(ssl_, data.data(), checked_cast(data.size())); const int ssl_error = SSL_get_error(ssl_, code); switch (ssl_error) { case SSL_ERROR_NONE: RTC_DLOG(LS_VERBOSE) << " -- success"; RTC_DCHECK_GT(code, 0); - RTC_DCHECK_LE(code, data_len); - if (read) { - *read = code; - } + RTC_DCHECK_LE(code, data.size()); + read = code; if (ssl_mode_ == SSL_MODE_DTLS) { // Enforce atomic reads -- this is a short read @@ -683,9 +699,7 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, if (pending) { RTC_DLOG(LS_INFO) << " -- short DTLS read. flushing"; FlushInput(pending); - if (error) { - *error = SSE_MSG_TRUNC; - } + error = SSE_MSG_TRUNC; return SR_ERROR; } } @@ -703,9 +717,7 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, return SR_EOS; default: Error("SSL_read", (ssl_error ? ssl_error : -1), 0, false); - if (error) { - *error = ssl_error_code_; - } + error = ssl_error_code_; return SR_ERROR; } // not reached diff --git a/rtc_base/openssl_stream_adapter.h b/rtc_base/openssl_stream_adapter.h index 891f0e6193..3c94ecd2ce 100644 --- a/rtc_base/openssl_stream_adapter.h +++ b/rtc_base/openssl_stream_adapter.h @@ -95,14 +95,19 @@ class OpenSSLStreamAdapter final : public SSLStreamAdapter { void SetMaxProtocolVersion(SSLProtocolVersion version) override; void SetInitialRetransmissionTimeout(int timeout_ms) override; - StreamResult Read(void* data, - size_t data_len, - size_t* read, - int* error) override; - StreamResult Write(const void* data, - size_t data_len, - size_t* written, - int* error) override; + [[deprecated("bugs.webrtc.org/14632")]] StreamResult + Read(void* data, size_t data_len, size_t* read, int* error) override; + StreamResult Read(rtc::ArrayView data, + size_t& read, + int& error) override; + [[deprecated("bugs.webrtc.org/14632")]] StreamResult Write( + const void* data, + size_t data_len, + size_t* written, + int* error) override; + StreamResult Write(rtc::ArrayView data, + size_t& written, + int& error) override; void Close() override; StreamState GetState() const override; diff --git a/rtc_base/ssl_adapter_unittest.cc b/rtc_base/ssl_adapter_unittest.cc index cd63249190..2da59ddbb2 100644 --- a/rtc_base/ssl_adapter_unittest.cc +++ b/rtc_base/ssl_adapter_unittest.cc @@ -204,7 +204,9 @@ class SSLAdapterTestDummyServer : public sigslot::has_slots<> { int error; rtc::StreamResult r = ssl_stream_adapter_->Write( - message.data(), message.length(), &written, &error); + rtc::MakeArrayView(reinterpret_cast(message.data()), + message.size()), + written, error); if (r == rtc::SR_SUCCESS) { return written; } else { @@ -236,18 +238,19 @@ class SSLAdapterTestDummyServer : public sigslot::has_slots<> { void OnSSLStreamAdapterEvent(rtc::StreamInterface* stream, int sig, int err) { if (sig & rtc::SE_READ) { - char buffer[4096] = ""; + uint8_t buffer[4096] = ""; size_t read; int error; // Read data received from the client and store it in our internal // buffer. - rtc::StreamResult r = - stream->Read(buffer, sizeof(buffer) - 1, &read, &error); + rtc::StreamResult r = stream->Read(buffer, read, error); if (r == rtc::SR_SUCCESS) { buffer[read] = '\0'; - RTC_LOG(LS_INFO) << "Server received '" << buffer << "'"; - data_ += buffer; + // Here we assume that the buffer is interpretable as string. + char* buffer_as_char = reinterpret_cast(buffer); + RTC_LOG(LS_INFO) << "Server received '" << buffer_as_char << "'"; + data_ += buffer_as_char; } } } diff --git a/rtc_base/ssl_stream_adapter_unittest.cc b/rtc_base/ssl_stream_adapter_unittest.cc index f6a7e323de..f8a015dc04 100644 --- a/rtc_base/ssl_stream_adapter_unittest.cc +++ b/rtc_base/ssl_stream_adapter_unittest.cc @@ -17,6 +17,7 @@ #include "absl/memory/memory.h" #include "absl/strings/string_view.h" +#include "api/array_view.h" #include "api/task_queue/pending_task_safety_flag.h" #include "rtc_base/buffer_queue.h" #include "rtc_base/checks.h" @@ -165,7 +166,9 @@ class SSLDummyStreamBase : public rtc::StreamInterface, int* error) override { rtc::StreamResult r; - r = in_->Read(buffer, buffer_len, read, error); + r = in_->Read( + rtc::MakeArrayView(reinterpret_cast(buffer), buffer_len), + *read, *error); if (r == rtc::SR_BLOCK) return rtc::SR_BLOCK; if (r == rtc::SR_EOS) @@ -201,17 +204,15 @@ class SSLDummyStreamBase : public rtc::StreamInterface, } // Write to the outgoing FifoBuffer - rtc::StreamResult WriteData(const void* data, - size_t data_len, - size_t* written, - int* error) { - return out_->Write(data, data_len, written, error); + rtc::StreamResult WriteData(rtc::ArrayView data, + size_t& written, + int& error) { + return out_->Write(data, written, error); } - rtc::StreamResult Write(const void* data, - size_t data_len, - size_t* written, - int* error) override; + rtc::StreamResult Write(rtc::ArrayView data, + size_t& written, + int& error) override; void Close() override { RTC_LOG(LS_INFO) << "Closing outbound stream"; @@ -649,17 +650,17 @@ class SSLStreamAdapterTestBase : public ::testing::Test, rtc::StreamResult DataWritten(SSLDummyStreamBase* from, const void* data, size_t data_len, - size_t* written, - int* error) { + size_t& written, + int& error) { // Randomly drop loss_ percent of packets if (rtc::CreateRandomId() % 100 < static_cast(loss_)) { RTC_LOG(LS_VERBOSE) << "Randomly dropping packet, size=" << data_len; - *written = data_len; + written = data_len; return rtc::SR_SUCCESS; } if (dtls_ && (data_len > mtu_)) { RTC_LOG(LS_VERBOSE) << "Dropping packet > mtu, size=" << data_len; - *written = data_len; + written = data_len; return rtc::SR_SUCCESS; } @@ -667,17 +668,19 @@ class SSLStreamAdapterTestBase : public ::testing::Test, // handshake packets and we damage the last byte to keep the header // intact but break the MAC. if (damage_ && (*static_cast(data) == 23)) { - std::vector buf(data_len); + std::vector buf(data_len); RTC_LOG(LS_VERBOSE) << "Damaging packet"; memcpy(&buf[0], data, data_len); buf[data_len - 1]++; - - return from->WriteData(&buf[0], data_len, written, error); + return from->WriteData(rtc::MakeArrayView(&buf[0], data_len), written, + error); } - return from->WriteData(data, data_len, written, error); + return from->WriteData( + rtc::MakeArrayView(reinterpret_cast(data), data_len), + written, error); } void SetDelay(int delay) { delay_ = delay; } @@ -838,7 +841,7 @@ class SSLStreamAdapterTestTLS size_t position, tosend, size; rtc::StreamResult rv; size_t sent; - char block[kBlockSize]; + uint8_t block[kBlockSize]; send_stream_.GetSize(&size); if (!size) @@ -848,7 +851,8 @@ class SSLStreamAdapterTestTLS send_stream_.GetPosition(&position); if (send_stream_.Read(block, sizeof(block), &tosend, nullptr) != rtc::SR_EOS) { - rv = client_ssl_->Write(block, tosend, &sent, 0); + int error; + rv = client_ssl_->Write(rtc::MakeArrayView(block, tosend), sent, error); if (rv == rtc::SR_SUCCESS) { send_stream_.SetPosition(position + sent); @@ -871,13 +875,13 @@ class SSLStreamAdapterTestTLS } void ReadData(rtc::StreamInterface* stream) override { - char buffer[1600]; + uint8_t buffer[1600]; size_t bread; int err2; rtc::StreamResult r; for (;;) { - r = stream->Read(buffer, sizeof(buffer), &bread, &err2); + r = stream->Read(buffer, bread, err2); if (r == rtc::SR_ERROR || r == rtc::SR_EOS) { // Unfortunately, errors are the way that the stream adapter @@ -930,7 +934,7 @@ class SSLStreamAdapterTestDTLSBase : public SSLStreamAdapterTestBase { } void WriteData() override { - unsigned char* packet = new unsigned char[1600]; + uint8_t* packet = new uint8_t[1600]; while (sent_ < count_) { unsigned int rand_state = sent_; @@ -942,7 +946,9 @@ class SSLStreamAdapterTestDTLSBase : public SSLStreamAdapterTestBase { } size_t sent; - rtc::StreamResult rv = client_ssl_->Write(packet, packet_size_, &sent, 0); + int error; + rtc::StreamResult rv = client_ssl_->Write( + rtc::MakeArrayView(packet, packet_size_), sent, error); if (rv == rtc::SR_SUCCESS) { RTC_LOG(LS_VERBOSE) << "Sent: " << sent_; sent_++; @@ -959,13 +965,13 @@ class SSLStreamAdapterTestDTLSBase : public SSLStreamAdapterTestBase { } void ReadData(rtc::StreamInterface* stream) override { - unsigned char buffer[2000]; + uint8_t buffer[2000]; size_t bread; int err2; rtc::StreamResult r; for (;;) { - r = stream->Read(buffer, 2000, &bread, &err2); + r = stream->Read(buffer, bread, err2); if (r == rtc::SR_ERROR) { // Unfortunately, errors are the way that the stream adapter @@ -1037,22 +1043,22 @@ class SSLStreamAdapterTestDTLS : SSLStreamAdapterTestDTLSBase(cert_pem, private_key_pem) {} }; -rtc::StreamResult SSLDummyStreamBase::Write(const void* data, - size_t data_len, - size_t* written, - int* error) { - RTC_LOG(LS_VERBOSE) << "Writing to loopback " << data_len; +rtc::StreamResult SSLDummyStreamBase::Write(rtc::ArrayView data, + size_t& written, + int& error) { + RTC_LOG(LS_VERBOSE) << "Writing to loopback " << data.size(); if (first_packet_) { first_packet_ = false; if (test_base_->GetLoseFirstPacket()) { - RTC_LOG(LS_INFO) << "Losing initial packet of length " << data_len; - *written = data_len; // Fake successful writing also to writer. + RTC_LOG(LS_INFO) << "Losing initial packet of length " << data.size(); + written = data.size(); // Fake successful writing also to writer. return rtc::SR_SUCCESS; } } - return test_base_->DataWritten(this, data, data_len, written, error); + return test_base_->DataWritten(this, data.data(), data.size(), written, + error); } class SSLStreamAdapterTestDTLSFromPEMStrings : public SSLStreamAdapterTestDTLS { @@ -1169,15 +1175,16 @@ TEST_P(SSLStreamAdapterTestTLS, ReadWriteAfterClose) { client_ssl_->Close(); rtc::StreamResult rv; - char block[kBlockSize]; + uint8_t block[kBlockSize]; size_t dummy; + int error; // It's an error to write after closed. - rv = client_ssl_->Write(block, sizeof(block), &dummy, nullptr); + rv = client_ssl_->Write(block, dummy, error); ASSERT_EQ(rtc::SR_ERROR, rv); // But after closed read gives you EOS. - rv = client_ssl_->Read(block, sizeof(block), &dummy, nullptr); + rv = client_ssl_->Read(block, dummy, error); ASSERT_EQ(rtc::SR_EOS, rv); } diff --git a/rtc_base/stream.cc b/rtc_base/stream.cc index e1aab8cc22..e6b74b49ac 100644 --- a/rtc_base/stream.cc +++ b/rtc_base/stream.cc @@ -15,6 +15,7 @@ #include #include +#include "api/array_view.h" #include "rtc_base/checks.h" #include "rtc_base/thread.h" @@ -31,8 +32,10 @@ StreamResult StreamInterface::WriteAll(const void* data, StreamResult result = SR_SUCCESS; size_t total_written = 0, current_written; while (total_written < data_len) { - result = Write(static_cast(data) + total_written, - data_len - total_written, ¤t_written, error); + result = Write(ArrayView( + reinterpret_cast(data) + total_written, + data_len - total_written), + current_written, *error); if (result != SR_SUCCESS) break; total_written += current_written; diff --git a/rtc_base/stream.h b/rtc_base/stream.h index c57ee61fcc..fa928fc9ae 100644 --- a/rtc_base/stream.h +++ b/rtc_base/stream.h @@ -69,24 +69,48 @@ class RTC_EXPORT StreamInterface { // block, or the stream is in SS_OPENING state. // SR_EOS: the end-of-stream has been reached, or the stream is in the // SS_CLOSED state. - virtual StreamResult Read(void* buffer, - size_t buffer_len, - size_t* read, - int* error) = 0; + + // The deprecated method has a default implementation that may be + // overridden in subclasses, rather than being =0. + // This allows subclasses to delete the method. + // TODO(bugs.webrtc.org/14632): Remove when downstream is converted. + [[deprecated("Use ArrayView version")]] virtual StreamResult + Read(void* buffer, size_t buffer_len, size_t* read, int* error) { + RTC_CHECK_NOTREACHED(); + } + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + // Preserve backwards compatibility using a default implementation + // because there are subclasses + // outside of the WebRTC codebase that need to be converted. + // + // TODO(bugs.webrtc.org/14632): Remove when downstream is converted. virtual StreamResult Read(rtc::ArrayView buffer, size_t& read, int& error) { return Read(buffer.data(), buffer.size(), &read, &error); } - virtual StreamResult Write(const void* data, - size_t data_len, - size_t* written, - int* error) = 0; +#pragma clang diagnostic pop + + // The deprecated method has a default implementation that may be + // overridden in subclasses, rather than being =0. + // This allows subclasses to delete the method. + // TODO(bugs.webrtc.org/14632): Remove when downstream is converted. + [[deprecated("Use ArrayView version")]] virtual StreamResult + Write(const void* data, size_t data_len, size_t* written, int* error) { + RTC_CHECK_NOTREACHED(); + } + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" virtual StreamResult Write(rtc::ArrayView data, size_t& written, int& error) { return Write(data.data(), data.size(), &written, &error); } +#pragma clang diagnostic pop + // Attempt to transition to the SS_CLOSED state. SE_CLOSE will not be // signalled as a result of this call. virtual void Close() = 0; @@ -115,14 +139,19 @@ class RTC_EXPORT StreamInterface { // unlike Write, the argument 'written' is always set, and may be non-zero // on results other than SR_SUCCESS. The remaining arguments have the // same semantics as Write. - StreamResult WriteAll(const void* data, - size_t data_len, - size_t* written, - int* error); + [[deprecated("Use version with ArrayView")]] StreamResult + WriteAll(const void* data, size_t data_len, size_t* written, int* error); - StreamResult WriteAll(ArrayView data, size_t& written, int& error) { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + // TODO(bugs.webrc.org/14632): Remove pragmas and change underlying + // implementation when downstream code is converted. + StreamResult WriteAll(ArrayView data, + size_t& written, + int& error) { return WriteAll(data.data(), data.size(), &written, &error); } +#pragma clang diagnostic pop protected: StreamInterface();