Skip to content

Commit 3bff109

Browse files
coryancoryan
committed
fix(storage): avoid crashes after move
The `storage::Object*Stream` classes should not crash when used after moved-from. They do not guarantee what value they hold, but crashing on functions that just query their state is unexpected. Note that we have not defined what pre-conditions are required by each function, so we could have decided to make it UB to call these functions. That seems too hostile when "not crash but returns some undefined value" is less hostile and about as easy to implement.
1 parent 942c723 commit 3bff109

3 files changed

Lines changed: 62 additions & 25 deletions

File tree

‎google/cloud/storage/object_read_stream.cc‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,20 @@ namespace google {
2020
namespace cloud {
2121
namespace storage {
2222
inline namespace STORAGE_CLIENT_NS {
23+
namespace {
24+
std::unique_ptr<internal::ObjectReadStreambuf> MakeErrorStreambuf() {
25+
return absl::make_unique<internal::ObjectReadStreambuf>(
26+
internal::ReadObjectRangeRequest("", ""),
27+
Status(StatusCode::kUnimplemented, "null stream"));
28+
}
29+
} // namespace
30+
2331
static_assert(std::is_move_assignable<ObjectReadStream>::value,
2432
"storage::ObjectReadStream must be move assignable.");
2533
static_assert(std::is_move_constructible<ObjectReadStream>::value,
2634
"storage::ObjectReadStream must be move constructible.");
2735

28-
ObjectReadStream::ObjectReadStream()
29-
: ObjectReadStream(absl::make_unique<internal::ObjectReadStreambuf>(
30-
internal::ReadObjectRangeRequest("", ""),
31-
Status(StatusCode::kUnimplemented, "null stream"))) {}
36+
ObjectReadStream::ObjectReadStream() : ObjectReadStream(MakeErrorStreambuf()) {}
3237

3338
ObjectReadStream::ObjectReadStream(ObjectReadStream&& rhs) noexcept
3439
: std::basic_istream<char>(std::move(rhs)),
@@ -42,7 +47,8 @@ ObjectReadStream::ObjectReadStream(ObjectReadStream&& rhs) noexcept
4247
// that derived classes can define their own move constructor and move
4348
// assignment.
4449
buf_(std::move(rhs.buf_)) {
45-
rhs.set_rdbuf(nullptr);
50+
rhs.buf_ = MakeErrorStreambuf();
51+
rhs.set_rdbuf(rhs.buf_.get());
4652
set_rdbuf(buf_.get());
4753
}
4854

‎google/cloud/storage/object_stream_test.cc‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,68 +24,89 @@ namespace storage {
2424
inline namespace STORAGE_CLIENT_NS {
2525
namespace {
2626

27+
using ::google::cloud::testing_util::IsOk;
2728
using ::google::cloud::testing_util::StatusIs;
29+
using ::testing::NotNull;
2830

2931
TEST(ObjectStream, ReadMoveConstructor) {
3032
ObjectReadStream reader;
33+
ASSERT_THAT(reader.rdbuf(), NotNull());
3134
reader.setstate(std::ios::badbit | std::ios::eofbit);
3235
EXPECT_TRUE(reader.bad());
3336
EXPECT_TRUE(reader.eof());
34-
EXPECT_NE(nullptr, reader.rdbuf());
3537

3638
ObjectReadStream copy(std::move(reader));
39+
ASSERT_THAT(copy.rdbuf(), NotNull());
3740
EXPECT_TRUE(copy.bad());
3841
EXPECT_TRUE(copy.eof());
3942
EXPECT_THAT(copy.status(), StatusIs(StatusCode::kUnimplemented));
4043

41-
EXPECT_EQ(nullptr, reader.rdbuf()); // NOLINT(bugprone-use-after-move)
42-
EXPECT_NE(nullptr, copy.rdbuf());
44+
// NOLINTNEXTLINE(bugprone-use-after-move)
45+
EXPECT_THAT(reader.status(), Not(IsOk()));
46+
EXPECT_THAT(reader.rdbuf(), NotNull());
4347
}
4448

4549
TEST(ObjectStream, ReadMoveAssignment) {
4650
ObjectReadStream reader;
51+
ASSERT_THAT(reader.rdbuf(), NotNull());
4752
reader.setstate(std::ios::badbit | std::ios::eofbit);
48-
EXPECT_NE(nullptr, reader.rdbuf());
4953

5054
ObjectReadStream copy;
5155

5256
copy = std::move(reader);
57+
ASSERT_THAT(copy.rdbuf(), NotNull());
5358
EXPECT_TRUE(copy.bad());
5459
EXPECT_TRUE(copy.eof());
5560
EXPECT_THAT(copy.status(), StatusIs(StatusCode::kUnimplemented));
5661

57-
EXPECT_EQ(nullptr, reader.rdbuf()); // NOLINT(bugprone-use-after-move)
58-
EXPECT_NE(nullptr, copy.rdbuf());
62+
// NOLINTNEXTLINE(bugprone-use-after-move)
63+
ASSERT_THAT(reader.rdbuf(), NotNull());
64+
EXPECT_THAT(reader.status(), Not(IsOk()));
5965
}
6066

6167
TEST(ObjectStream, WriteMoveConstructor) {
6268
ObjectWriteStream writer;
69+
ASSERT_THAT(writer.rdbuf(), NotNull());
6370
EXPECT_THAT(writer.metadata(), StatusIs(StatusCode::kUnimplemented));
64-
EXPECT_NE(nullptr, writer.rdbuf());
6571

6672
ObjectWriteStream copy(std::move(writer));
73+
ASSERT_THAT(copy.rdbuf(), NotNull());
6774
EXPECT_TRUE(copy.bad());
6875
EXPECT_TRUE(copy.eof());
6976
EXPECT_THAT(copy.metadata(), StatusIs(StatusCode::kUnimplemented));
7077

71-
EXPECT_EQ(nullptr, writer.rdbuf()); // NOLINT(bugprone-use-after-move)
72-
EXPECT_NE(nullptr, copy.rdbuf());
78+
// NOLINTNEXTLINE(bugprone-use-after-move)
79+
ASSERT_THAT(writer.rdbuf(), NotNull());
80+
EXPECT_THAT(writer.last_status(), Not(IsOk()));
7381
}
7482

7583
TEST(ObjectStream, WriteMoveAssignment) {
7684
ObjectWriteStream writer;
85+
ASSERT_THAT(writer.rdbuf(), NotNull());
7786
EXPECT_THAT(writer.metadata(), StatusIs(StatusCode::kUnimplemented));
78-
EXPECT_NE(nullptr, writer.rdbuf());
7987

8088
ObjectWriteStream copy;
8189

8290
copy = std::move(writer);
91+
ASSERT_THAT(copy.rdbuf(), NotNull());
8392
EXPECT_TRUE(copy.bad());
8493
EXPECT_TRUE(copy.eof());
8594
EXPECT_THAT(copy.metadata(), StatusIs(StatusCode::kUnimplemented));
8695

87-
EXPECT_EQ(nullptr, writer.rdbuf()); // NOLINT(bugprone-use-after-move)
88-
EXPECT_NE(nullptr, copy.rdbuf());
96+
// NOLINTNEXTLINE(bugprone-use-after-move)
97+
ASSERT_THAT(writer.rdbuf(), NotNull());
98+
EXPECT_THAT(writer.last_status(), Not(IsOk()));
99+
}
100+
101+
TEST(ObjectStream, Suspend) {
102+
ObjectWriteStream writer;
103+
ASSERT_THAT(writer.rdbuf(), NotNull());
104+
EXPECT_THAT(writer.metadata(), StatusIs(StatusCode::kUnimplemented));
105+
106+
std::move(writer).Suspend();
107+
// NOLINTNEXTLINE(bugprone-use-after-move)
108+
ASSERT_THAT(writer.rdbuf(), NotNull());
109+
EXPECT_THAT(writer.last_status(), Not(IsOk()));
89110
}
90111

91112
} // namespace

‎google/cloud/storage/object_write_stream.cc‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,23 @@ namespace google {
2121
namespace cloud {
2222
namespace storage {
2323
inline namespace STORAGE_CLIENT_NS {
24+
namespace {
25+
std::unique_ptr<internal::ObjectWriteStreambuf> MakeErrorStreambuf() {
26+
return absl::make_unique<internal::ObjectWriteStreambuf>(
27+
absl::make_unique<internal::ResumableUploadSessionError>(
28+
Status(StatusCode::kUnimplemented, "null stream")),
29+
/*max_buffer_size=*/0, internal::CreateNullHashFunction(),
30+
internal::HashValues{}, internal::CreateNullHashValidator(),
31+
AutoFinalizeConfig::kDisabled);
32+
}
33+
} // namespace
2434
static_assert(std::is_move_assignable<ObjectWriteStream>::value,
2535
"storage::ObjectWriteStream must be move assignable.");
2636
static_assert(std::is_move_constructible<ObjectWriteStream>::value,
2737
"storage::ObjectWriteStream must be move constructible.");
2838

2939
ObjectWriteStream::ObjectWriteStream()
30-
: ObjectWriteStream(absl::make_unique<internal::ObjectWriteStreambuf>(
31-
absl::make_unique<internal::ResumableUploadSessionError>(
32-
Status(StatusCode::kUnimplemented, "null stream")),
33-
/*max_buffer_size=*/0, internal::CreateNullHashFunction(),
34-
internal::HashValues{}, internal::CreateNullHashValidator(),
35-
AutoFinalizeConfig::kDisabled)) {}
40+
: ObjectWriteStream(MakeErrorStreambuf()) {}
3641

3742
ObjectWriteStream::ObjectWriteStream(
3843
std::unique_ptr<internal::ObjectWriteStreambuf> buf)
@@ -57,7 +62,8 @@ ObjectWriteStream::ObjectWriteStream(ObjectWriteStream&& rhs) noexcept
5762
metadata_(std::move(rhs.metadata_)),
5863
headers_(std::move(rhs.headers_)),
5964
payload_(std::move(rhs.payload_)) {
60-
rhs.set_rdbuf(nullptr);
65+
rhs.buf_ = MakeErrorStreambuf();
66+
rhs.set_rdbuf(rhs.buf_.get());
6167
set_rdbuf(buf_.get());
6268
if (!buf_) {
6369
setstate(std::ios::badbit | std::ios::eofbit);
@@ -96,7 +102,11 @@ void ObjectWriteStream::CloseBuf() {
96102
}
97103
}
98104

99-
void ObjectWriteStream::Suspend() && { buf_.reset(); }
105+
void ObjectWriteStream::Suspend() && {
106+
ObjectWriteStream tmp;
107+
swap(tmp);
108+
tmp.buf_.reset();
109+
}
100110

101111
} // namespace STORAGE_CLIENT_NS
102112
} // namespace storage

0 commit comments

Comments
 (0)