Skip to content

Commit f5e14cf

Browse files
authored
fix: fix segmentation fault due to the use of an invalid grpc server_ object.
BuildAndStart() can fail for any number of reasons (e.g. when passed a port reserved for privileged use or a non-existent local address). We were not checking for this and would try to use the server unconditionally, causing a segmentation fault (e.g. when emulator was called by a non-root user as: ./emulator -p 100). This adds a fix so that we can check that we have a valid server object before trying to use it. Also added a unit test for the failing path. --------- Signed-off-by: Brian Gitonga Marete <bgmarete@gmail.com>
1 parent fd7f0fe commit f5e14cf

4 files changed

Lines changed: 35 additions & 6 deletions

File tree

google/cloud/bigtable/emulator/emulator.cc

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,17 @@ int main(int argc, char* argv[]) {
3333
absl::StrCat("Usage: %s -h <host> -p <port>", argv[0]));
3434
absl::ParseCommandLine(argc, argv);
3535

36-
auto server = google::cloud::bigtable::emulator::CreateDefaultEmulatorServer(
37-
absl::GetFlag(FLAGS_host), absl::GetFlag(FLAGS_port));
36+
auto maybe_server =
37+
google::cloud::bigtable::emulator::CreateDefaultEmulatorServer(
38+
absl::GetFlag(FLAGS_host), absl::GetFlag(FLAGS_port));
39+
if (!maybe_server) {
40+
std::cerr << "CreateDefaultEmulatorServer() failed. See logs for "
41+
"possible reason"
42+
<< std::endl;
43+
return 1;
44+
}
45+
46+
auto& server = maybe_server.value();
3847

3948
std::cout << "Server running on port " << server->bound_port() << "\n";
4049
server->Wait();

google/cloud/bigtable/emulator/server.cc

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@ class DefaultEmulatorServer : public EmulatorServer {
333333
int bound_port() override { return bound_port_; }
334334
void Shutdown() override { server_->Shutdown(); }
335335
void Wait() override { server_->Wait(); }
336+
bool HasValidServer() { return static_cast<bool>(server_); }
336337

337338
private:
338339
int bound_port_;
@@ -343,9 +344,17 @@ class DefaultEmulatorServer : public EmulatorServer {
343344
std::unique_ptr<grpc::Server> server_;
344345
};
345346

346-
std::unique_ptr<EmulatorServer> CreateDefaultEmulatorServer(
347+
StatusOr<std::unique_ptr<EmulatorServer>> CreateDefaultEmulatorServer(
347348
std::string const& host, std::uint16_t port) {
348-
return std::unique_ptr<EmulatorServer>(new DefaultEmulatorServer(host, port));
349+
auto* default_emulator_server = new DefaultEmulatorServer(host, port);
350+
if (!default_emulator_server->HasValidServer()) {
351+
return UnknownError("An unknown error occurred when starting server",
352+
GCP_ERROR_INFO()
353+
.WithMetadata("host", host)
354+
.WithMetadata("port", absl::StrCat("%d", port)));
355+
}
356+
357+
return std::unique_ptr<EmulatorServer>(default_emulator_server);
349358
}
350359

351360
} // namespace emulator

google/cloud/bigtable/emulator/server.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_BIGTABLE_EMULATOR_SERVER_H
1616
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_BIGTABLE_EMULATOR_SERVER_H
1717

18+
#include "google/cloud/status_or.h"
1819
#include <cstdint>
1920
#include <memory>
2021
#include <string>
@@ -36,7 +37,7 @@ class EmulatorServer {
3637
virtual void Wait() = 0;
3738
};
3839

39-
std::unique_ptr<EmulatorServer> CreateDefaultEmulatorServer(
40+
StatusOr<std::unique_ptr<EmulatorServer>> CreateDefaultEmulatorServer(
4041
std::string const& host, std::uint16_t port);
4142

4243
} // namespace emulator

google/cloud/bigtable/emulator/server_test.cc

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,15 @@
1313
// limitations under the License.
1414

1515
#include "google/cloud/bigtable/emulator/server.h"
16+
#include "google/cloud/testing_util/status_matchers.h"
1617
#include <google/bigtable/admin/v2/bigtable_table_admin.grpc.pb.h>
1718
#include <google/bigtable/admin/v2/bigtable_table_admin.pb.h>
1819
#include <google/bigtable/admin/v2/table.pb.h>
1920
#include <google/bigtable/v2/bigtable.grpc.pb.h>
2021
#include <google/bigtable/v2/bigtable.pb.h>
2122
#include <gmock/gmock.h>
2223
#include <grpcpp/grpcpp.h>
24+
#include <gtest/gtest.h>
2325

2426
namespace google {
2527
namespace cloud {
@@ -33,7 +35,9 @@ class ServerTest : public ::testing::Test {
3335
grpc::ClientContext ctx_;
3436

3537
void SetUp() override {
36-
server_ = CreateDefaultEmulatorServer("127.0.0.1", 0);
38+
auto maybe_server = CreateDefaultEmulatorServer("127.0.0.1", 0);
39+
ASSERT_STATUS_OK(maybe_server);
40+
server_ = std::move(maybe_server.value());
3741
channel_ = grpc::CreateChannel(
3842
"localhost:" + std::to_string(server_->bound_port()),
3943
grpc::InsecureChannelCredentials());
@@ -215,6 +219,12 @@ TEST_F(ServerTest, TableAdminUpdateTable) {
215219
EXPECT_NE(status.error_code(), grpc::StatusCode::UNIMPLEMENTED);
216220
}
217221

222+
// Test that the failure path for server creation does not crash.
223+
TEST(ServerCreationTest, TestServerCreationFailurePath) {
224+
auto maybe_server = CreateDefaultEmulatorServer("invalid_host_address", 0);
225+
ASSERT_EQ(false, maybe_server.ok());
226+
}
227+
218228
} // namespace emulator
219229
} // namespace bigtable
220230
} // namespace cloud

0 commit comments

Comments
 (0)