Skip to content

Commit 7bb94f4

Browse files
Fix bad_weak_ptr when close a ClientConnection during construction (#350)
Fixes #348 Fixes #349 ### Motivation When `close` is called in `ClientConnection`'s constructor, `shared_from_this()` will be called, which results in a `std::bad_weak_ptr` error. This error does not happen before #317 because `shared_from_this()` could only be called when the `producers` or `consumers` field is not empty. ### Modifications Throw `ResultAuthenticationError` when there is anything wrong with authentication in `ClientConnection`'s constructor. Then catch the result in `ConnectionPool::getConnectionAsync` and returned the failed future. In addition, check `authentication_` even for non-TLS URLs. Otherwise, the null authentication will be used to construct `CommandConnect`. Add `testInvalidPlugin` and `testTlsConfigError` to verify the changes.
1 parent c771b12 commit 7bb94f4

File tree

4 files changed

+30
-15
lines changed

4 files changed

+30
-15
lines changed

lib/ClientConnection.cc

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,12 @@ ClientConnection::ClientConnection(const std::string& logicalAddress, const std:
187187
pool_(pool),
188188
poolIndex_(poolIndex) {
189189
LOG_INFO(cnxString_ << "Create ClientConnection, timeout=" << clientConfiguration.getConnectionTimeout());
190+
if (!authentication_) {
191+
LOG_ERROR("Invalid authentication plugin");
192+
throw ResultAuthenticationError;
193+
return;
194+
}
195+
190196
if (clientConfiguration.isUseTls()) {
191197
#if BOOST_VERSION >= 105400
192198
boost::asio::ssl::context ctx(boost::asio::ssl::context::tlsv12_client);
@@ -207,20 +213,13 @@ ClientConnection::ClientConnection(const std::string& logicalAddress, const std:
207213
ctx.load_verify_file(trustCertFilePath);
208214
} else {
209215
LOG_ERROR(trustCertFilePath << ": No such trustCertFile");
210-
close(ResultAuthenticationError, false);
211-
return;
216+
throw ResultAuthenticationError;
212217
}
213218
} else {
214219
ctx.set_default_verify_paths();
215220
}
216221
}
217222

218-
if (!authentication_) {
219-
LOG_ERROR("Invalid authentication plugin");
220-
close(ResultAuthenticationError, false);
221-
return;
222-
}
223-
224223
std::string tlsCertificates = clientConfiguration.getTlsCertificateFilePath();
225224
std::string tlsPrivateKey = clientConfiguration.getTlsPrivateKeyFilePath();
226225

@@ -231,13 +230,11 @@ ClientConnection::ClientConnection(const std::string& logicalAddress, const std:
231230
tlsPrivateKey = authData->getTlsPrivateKey();
232231
if (!file_exists(tlsCertificates)) {
233232
LOG_ERROR(tlsCertificates << ": No such tlsCertificates");
234-
close(ResultAuthenticationError, false);
235-
return;
233+
throw ResultAuthenticationError;
236234
}
237235
if (!file_exists(tlsCertificates)) {
238236
LOG_ERROR(tlsCertificates << ": No such tlsCertificates");
239-
close(ResultAuthenticationError, false);
240-
return;
237+
throw ResultAuthenticationError;
241238
}
242239
ctx.use_private_key_file(tlsPrivateKey, boost::asio::ssl::context::pem);
243240
ctx.use_certificate_file(tlsCertificates, boost::asio::ssl::context::pem);

lib/ClientConnection.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,7 @@ class PULSAR_PUBLIC ClientConnection : public std::enable_shared_from_this<Clien
148148
* @param result all pending futures will complete with this result
149149
* @param detach remove it from the pool if it's true
150150
*
151-
* `detach` should only be false when:
152-
* 1. Before the connection is put into the pool, i.e. during the construction.
153-
* 2. When the connection pool is closed
151+
* `detach` should only be false when the connection pool is closed.
154152
*/
155153
void close(Result result = ResultConnectError, bool detach = true);
156154

lib/ConnectionPool.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(const
9999
cnx.reset(new ClientConnection(logicalAddress, physicalAddress, executorProvider_->get(keySuffix),
100100
clientConfiguration_, authentication_, clientVersion_, *this,
101101
keySuffix));
102+
} catch (Result result) {
103+
Promise<Result, ClientConnectionWeakPtr> promise;
104+
promise.setFailed(result);
105+
return promise.getFuture();
102106
} catch (const std::runtime_error& e) {
103107
lock.unlock();
104108
LOG_ERROR("Failed to create connection: " << e.what())

tests/AuthPluginTest.cc

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,3 +581,19 @@ TEST(AuthPluginTest, testOauth2Failure) {
581581
ASSERT_EQ(client5.createProducer(topic, producer), ResultAuthenticationError);
582582
client5.close();
583583
}
584+
585+
TEST(AuthPluginTest, testInvalidPlugin) {
586+
Client client("pulsar://localhost:6650", ClientConfiguration{}.setAuth(AuthFactory::create("invalid")));
587+
Producer producer;
588+
ASSERT_EQ(ResultAuthenticationError, client.createProducer("my-topic", producer));
589+
client.close();
590+
}
591+
592+
TEST(AuthPluginTest, testTlsConfigError) {
593+
Client client(serviceUrlTls, ClientConfiguration{}
594+
.setAuth(AuthTls::create(clientPublicKeyPath, clientPrivateKeyPath))
595+
.setTlsTrustCertsFilePath("invalid"));
596+
Producer producer;
597+
ASSERT_EQ(ResultAuthenticationError, client.createProducer("my-topic", producer));
598+
client.close();
599+
}

0 commit comments

Comments
 (0)