From 6d96dff9f7bca0b03e85f678a8810d2e682f9038 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Mon, 21 Dec 2020 09:47:25 -0800 Subject: [PATCH] Changes CKeyPair::CopyFrom 1.) It doesn't return bool anymore. It just considers failure fatal. Very few call sites were checking it anyway, and all of the ones that were were just asserting anyway, not actually handling it. Sort of a classic example of a funciton where you should not try to handle failure. 2.) If the source key is empty, that is no longer considered an error. We just set the destination key to empty. Given that nobody was previously handling this error anyway, except for the people checking and asserting, I think this is safe. --- src/common/crypto_25519.h | 8 ++++---- src/common/keypair.cpp | 10 +++------- src/common/keypair.h | 2 +- .../clientlib/steamnetworkingsockets_connections.cpp | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/common/crypto_25519.h b/src/common/crypto_25519.h index fc62a25..08016b5 100644 --- a/src/common/crypto_25519.h +++ b/src/common/crypto_25519.h @@ -88,8 +88,8 @@ public: CECKeyExchangePublicKey() : CEC25519PublicKeyBase( k_ECryptoKeyTypeKeyExchangePublic ) { } // Allow copying of public keys without a bunch of paranoia. - CECKeyExchangePublicKey( const CECKeyExchangePublicKey &x ) : CEC25519PublicKeyBase( k_ECryptoKeyTypeKeyExchangePublic ) { VerifyFatal( CopyFrom( x ) ); } - CECKeyExchangePublicKey & operator=(const CECKeyExchangePublicKey &x) { if ( this != &x ) { VerifyFatal( CopyFrom( x ) ); } return *this; } + CECKeyExchangePublicKey( const CECKeyExchangePublicKey &x ) : CEC25519PublicKeyBase( k_ECryptoKeyTypeKeyExchangePublic ) { CopyFrom( x ); } + CECKeyExchangePublicKey & operator=(const CECKeyExchangePublicKey &x) { if ( this != &x ) { CopyFrom( x ); } return *this; } virtual ~CECKeyExchangePublicKey(); }; @@ -132,8 +132,8 @@ public: CECSigningPublicKey() : CEC25519PublicKeyBase( k_ECryptoKeyTypeSigningPublic ) { } // Allow copying of public keys without a bunch of paranoia. - CECSigningPublicKey( const CECSigningPublicKey &x ) : CEC25519PublicKeyBase( k_ECryptoKeyTypeSigningPublic ) { VerifyFatal( CopyFrom( x ) ); } - CECSigningPublicKey& operator=(const CECSigningPublicKey &x) { if ( this != &x ) { VerifyFatal( CopyFrom( x ) ); } return *this; } + CECSigningPublicKey( const CECSigningPublicKey &x ) : CEC25519PublicKeyBase( k_ECryptoKeyTypeSigningPublic ) { CopyFrom( x ); } + CECSigningPublicKey& operator=(const CECSigningPublicKey &x) { if ( this != &x ) { CopyFrom( x ); } return *this; } virtual bool LoadFromAndWipeBuffer( void *pBuffer, size_t cBytes ) override; diff --git a/src/common/keypair.cpp b/src/common/keypair.cpp index a4b19bd..1fee79c 100644 --- a/src/common/keypair.cpp +++ b/src/common/keypair.cpp @@ -364,21 +364,17 @@ bool CCryptoKeyBase::operator==( const CCryptoKeyBase &rhs ) const return memcmp( bufLHS.Base(), bufRHS.Base(), cbRawData ) == 0; } - -bool CCryptoKeyBase::CopyFrom( const CCryptoKeyBase &x ) +void CCryptoKeyBase::CopyFrom( const CCryptoKeyBase &x ) { Assert( m_eKeyType == x.m_eKeyType ); Wipe(); uint32 cbData = x.GetRawData( nullptr ); if ( cbData == 0 ) - { - Assert( false ); - return false; - } + return; void *tmp = alloca( cbData ); VerifyFatal( x.GetRawData( tmp ) == cbData ); - return SetRawDataAndWipeInput( tmp, cbData ); + VerifyFatal( SetRawDataAndWipeInput( tmp, cbData ) ); } bool CCryptoKeyBase::LoadFromAndWipeBuffer( void *pBuffer, size_t cBytes ) diff --git a/src/common/keypair.h b/src/common/keypair.h index 6c1d2d9..fd8fdf8 100644 --- a/src/common/keypair.h +++ b/src/common/keypair.h @@ -84,7 +84,7 @@ public: bool operator!=( const CCryptoKeyBase &rhs ) const { return !operator==( rhs ); } // Make a copy of the key, by using the raw data functions - bool CopyFrom( const CCryptoKeyBase &x ); + void CopyFrom( const CCryptoKeyBase &x ); #ifdef DBGFLAG_VALIDATE virtual void Validate( CValidator &validator, const char *pchName ) const = 0; // Validate our internal structures diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp index a174aa3..1b03abe 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp @@ -1134,7 +1134,7 @@ void CSteamNetworkConnectionBase::SetLocalCert( const CMsgSteamDatagramCertifica // but we'll only keep this around for a brief time. It's possible for the // interface to get a new cert (with a new private key) while we are starting this // connection. We'll keep using the old one, which may be totally valid. - DbgVerify( m_keyPrivate.CopyFrom( keyPrivate ) ); + m_keyPrivate.CopyFrom( keyPrivate ); // Save off the signed certificate m_msgSignedCertLocal = msgSignedCert;