Skip to content

Commit d4371b6

Browse files
JonathanHensonJonathanHenson
authored
Fixed bug in Aws::Crt::Optional causing copies to cause memory errors. (aws#29)
* Fixed bug in Aws::Crt::Optional causing copies to cause memory errors.
1 parent fa59298 commit d4371b6

File tree

7 files changed

+215
-20
lines changed

7 files changed

+215
-20
lines changed

‎include/aws/crt/JsonObject.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ namespace Aws
5050
* Moves the ownership of the internal JSON DOM.
5151
* No copying is performed.
5252
*/
53-
JsonObject(JsonObject &&value);
53+
JsonObject(JsonObject &&value) noexcept;
5454

5555
~JsonObject();
5656

@@ -66,7 +66,7 @@ namespace Aws
6666
* @warning This will result in invalidating any outstanding views of the current DOM. However, views
6767
* to the moved-from DOM would still valid.
6868
*/
69-
JsonObject &operator=(JsonObject &&other);
69+
JsonObject &operator=(JsonObject &&other) noexcept;
7070

7171
bool operator==(const JsonObject &other) const;
7272
bool operator!=(const JsonObject &other) const;

‎include/aws/crt/Optional.h

Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ namespace Aws
5656
{
5757
if (m_value)
5858
{
59-
m_value->~T();
59+
*m_value = std::forward<U>(u);
60+
return *this;
6061
}
6162

6263
new (&m_storage) T(std::forward<U>(u));
@@ -65,7 +66,18 @@ namespace Aws
6566
return *this;
6667
}
6768

68-
Optional(const Optional<T> &other) = default;
69+
Optional(const Optional<T> &other)
70+
{
71+
if (other.m_value)
72+
{
73+
new (&m_storage) T(*other.m_value);
74+
m_value = reinterpret_cast<T *>(&m_storage);
75+
}
76+
else
77+
{
78+
m_value = nullptr;
79+
}
80+
}
6981

7082
Optional(Optional<T> &&other)
7183
{
@@ -74,10 +86,13 @@ namespace Aws
7486
new (&m_storage) T(std::forward<T>(*other.m_value));
7587
m_value = reinterpret_cast<T *>(&m_storage);
7688
}
77-
other.m_value = nullptr;
89+
else
90+
{
91+
m_value = nullptr;
92+
}
7893
}
7994

80-
template <typename U = T> Optional<T> &operator=(const Optional<U> &other)
95+
Optional &operator=(const Optional &other)
8196
{
8297
if (this == &other)
8398
{
@@ -86,18 +101,54 @@ namespace Aws
86101

87102
if (m_value)
88103
{
89-
m_value->~T();
104+
if (other.m_value)
105+
{
106+
*m_value = *other.m_value;
107+
}
108+
else
109+
{
110+
m_value->~T();
111+
m_value = nullptr;
112+
}
113+
114+
return *this;
90115
}
91116

92117
if (other.m_value)
93118
{
94119
new (&m_storage) T(*other.m_value);
95120
m_value = reinterpret_cast<T *>(&m_storage);
96-
other.m_value = nullptr;
97121
}
98-
else
122+
123+
return *this;
124+
}
125+
126+
template <typename U = T> Optional<T> &operator=(const Optional<U> &other)
127+
{
128+
if (this == &other)
99129
{
100-
m_value = nullptr;
130+
return *this;
131+
}
132+
133+
if (m_value)
134+
{
135+
if (other.m_value)
136+
{
137+
*m_value = *other.m_value;
138+
}
139+
else
140+
{
141+
m_value->~T();
142+
m_value = nullptr;
143+
}
144+
145+
return *this;
146+
}
147+
148+
if (other.m_value)
149+
{
150+
new (&m_storage) T(*other.m_value);
151+
m_value = reinterpret_cast<T *>(&m_storage);
101152
}
102153

103154
return *this;
@@ -112,18 +163,23 @@ namespace Aws
112163

113164
if (m_value)
114165
{
115-
m_value->~T();
166+
if (other.m_value)
167+
{
168+
*m_value = std::forward(*other.m_value);
169+
}
170+
else
171+
{
172+
m_value->~T();
173+
m_value = nullptr;
174+
}
175+
176+
return *this;
116177
}
117178

118179
if (other.m_value)
119180
{
120181
new (&m_storage) T(std::forward<U>(*other.m_value));
121182
m_value = reinterpret_cast<T *>(&m_storage);
122-
other.m_value = nullptr;
123-
}
124-
else
125-
{
126-
m_value = nullptr;
127183
}
128184

129185
return *this;

‎samples/mqtt_pub_sub/main.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ int main(int argc, char *argv[])
253253
conditionVariable.notify_one();
254254
};
255255

256-
connection->Subscribe(topic.c_str(), AWS_MQTT_QOS_AT_MOST_ONCE, onPublish, onSubAck);
256+
connection->Subscribe(topic.c_str(), AWS_MQTT_QOS_AT_LEAST_ONCE, onPublish, onSubAck);
257257
conditionVariable.wait(uniqueLock);
258258

259259
while (true)
@@ -286,7 +286,7 @@ int main(int argc, char *argv[])
286286
fprintf(stdout, "Operation failed with error %s\n", aws_error_debug_str(errorCode));
287287
}
288288
};
289-
connection->Publish(topic.c_str(), AWS_MQTT_QOS_AT_MOST_ONCE, false, payload, onPublishComplete);
289+
connection->Publish(topic.c_str(), AWS_MQTT_QOS_AT_LEAST_ONCE, false, payload, onPublishComplete);
290290
}
291291

292292
/*

‎source/JsonObject.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ namespace Aws
5050
{
5151
}
5252

53-
JsonObject::JsonObject(JsonObject &&value)
53+
JsonObject::JsonObject(JsonObject &&value) noexcept
5454
: m_value(value.m_value), m_wasParseSuccessful(value.m_wasParseSuccessful),
5555
m_errorMessage(std::move(value.m_errorMessage))
5656
{
@@ -75,7 +75,7 @@ namespace Aws
7575
return *this;
7676
}
7777

78-
JsonObject &JsonObject::operator=(JsonObject &&other)
78+
JsonObject &JsonObject::operator=(JsonObject &&other) noexcept
7979
{
8080
if (this == &other)
8181
{

‎tests/CMakeLists.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ add_test_case(HttpClientConnectionManagerResourceSafety)
2626
add_test_case(HttpClientConnectionWithPendingAcquisitions)
2727
add_test_case(HttpClientConnectionWithPendingAcquisitionsAndClosedConnections)
2828
add_test_case(DefaultResolution)
29+
add_test_case(OptionalCopySafety)
30+
add_test_case(OptionalMoveSafety)
31+
add_test_case(OptionalCopyAndMoveSemantics)
2932

3033
generate_cpp_test_driver(${TEST_BINARY_NAME})
3134

‎tests/JsonParserTest.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ static int s_JsonExplicitNullTest(struct aws_allocator *allocator, void *ctx)
103103
Aws::Crt::JsonObject doc;
104104
Aws::Crt::JsonObject nullObject;
105105
nullObject.AsNull();
106+
106107
doc.WithObject("testKey", nullObject);
107108

108109
auto str = doc.View().WriteCompact(true);

‎tests/OptionalMemorySafetyTest.cpp

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/*
2+
* Copyright 2010-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
#include <aws/crt/Api.h>
16+
#include <aws/crt/Optional.h>
17+
#include <aws/testing/aws_test_harness.h>
18+
19+
const char *s_test_str = "This is a string, that should be long enough to avoid small string optimizations";
20+
21+
static int s_OptionalCopySafety(struct aws_allocator *allocator, void *ctx)
22+
{
23+
(void)ctx;
24+
Aws::Crt::ApiHandle apiHandle(allocator);
25+
26+
Aws::Crt::Optional<Aws::Crt::String> str1(s_test_str);
27+
Aws::Crt::Optional<Aws::Crt::String> strCpyAssigned = str1;
28+
Aws::Crt::Optional<Aws::Crt::String> strCpyConstructedOptional(strCpyAssigned);
29+
Aws::Crt::Optional<Aws::Crt::String> strCpyConstructedValue(*strCpyAssigned);
30+
31+
// now force data access just to check there's not a segfault hiding somewhere.
32+
ASSERT_STR_EQUALS(s_test_str, str1->c_str());
33+
ASSERT_STR_EQUALS(s_test_str, strCpyAssigned->c_str());
34+
ASSERT_STR_EQUALS(s_test_str, strCpyConstructedOptional->c_str());
35+
ASSERT_STR_EQUALS(s_test_str, strCpyConstructedValue->c_str());
36+
37+
return AWS_OP_SUCCESS;
38+
}
39+
40+
AWS_TEST_CASE(OptionalCopySafety, s_OptionalCopySafety)
41+
42+
static int s_OptionalMoveSafety(struct aws_allocator *allocator, void *ctx)
43+
{
44+
(void)ctx;
45+
Aws::Crt::ApiHandle apiHandle(allocator);
46+
47+
Aws::Crt::Optional<Aws::Crt::String> str1(s_test_str);
48+
Aws::Crt::Optional<Aws::Crt::String> strMoveAssigned = std::move(str1);
49+
ASSERT_STR_EQUALS(s_test_str, strMoveAssigned->c_str());
50+
51+
Aws::Crt::Optional<Aws::Crt::String> strMoveValueAssigned = std::move(*strMoveAssigned);
52+
ASSERT_STR_EQUALS(s_test_str, strMoveValueAssigned->c_str());
53+
54+
Aws::Crt::Optional<Aws::Crt::String> strMoveConstructed(std::move(strMoveValueAssigned));
55+
ASSERT_STR_EQUALS(s_test_str, strMoveConstructed->c_str());
56+
57+
Aws::Crt::Optional<Aws::Crt::String> strMoveValueConstructed(std::move(*strMoveConstructed));
58+
ASSERT_STR_EQUALS(s_test_str, strMoveValueConstructed->c_str());
59+
60+
return AWS_OP_SUCCESS;
61+
}
62+
63+
AWS_TEST_CASE(OptionalMoveSafety, s_OptionalMoveSafety)
64+
65+
class CopyMoveTester
66+
{
67+
public:
68+
CopyMoveTester() : m_copied(false), m_moved(false) {}
69+
CopyMoveTester(const CopyMoveTester &other) : m_copied(true), m_moved(false) {}
70+
CopyMoveTester(CopyMoveTester &&other) : m_copied(false), m_moved(true) {}
71+
72+
CopyMoveTester &operator=(const CopyMoveTester &other)
73+
{
74+
m_copied = true;
75+
m_moved = false;
76+
return *this;
77+
}
78+
CopyMoveTester &operator=(CopyMoveTester &&other)
79+
{
80+
m_copied = false;
81+
m_moved = true;
82+
return *this;
83+
}
84+
85+
~CopyMoveTester() {}
86+
87+
bool m_copied;
88+
bool m_moved;
89+
};
90+
91+
static int s_OptionalCopyAndMoveSemantics(struct aws_allocator *allocator, void *ctx)
92+
{
93+
(void)ctx;
94+
Aws::Crt::ApiHandle apiHandle(allocator);
95+
96+
CopyMoveTester initialItem;
97+
ASSERT_FALSE(initialItem.m_copied);
98+
ASSERT_FALSE(initialItem.m_moved);
99+
100+
Aws::Crt::Optional<CopyMoveTester> copyConstructedValue(initialItem);
101+
ASSERT_TRUE(copyConstructedValue->m_copied);
102+
ASSERT_FALSE(copyConstructedValue->m_moved);
103+
104+
Aws::Crt::Optional<CopyMoveTester> copyConstructedOptional(copyConstructedValue);
105+
ASSERT_TRUE(copyConstructedOptional->m_copied);
106+
ASSERT_FALSE(copyConstructedOptional->m_moved);
107+
108+
Aws::Crt::Optional<CopyMoveTester> copyAssignedValue = initialItem;
109+
ASSERT_TRUE(copyAssignedValue->m_copied);
110+
ASSERT_FALSE(copyAssignedValue->m_moved);
111+
112+
Aws::Crt::Optional<CopyMoveTester> copyAssignedOptional = copyConstructedOptional;
113+
ASSERT_TRUE(copyAssignedOptional->m_copied);
114+
ASSERT_FALSE(copyAssignedOptional->m_moved);
115+
116+
Aws::Crt::Optional<CopyMoveTester> moveConstructedValue(std::move(initialItem));
117+
ASSERT_FALSE(moveConstructedValue->m_copied);
118+
ASSERT_TRUE(moveConstructedValue->m_moved);
119+
120+
Aws::Crt::Optional<CopyMoveTester> moveConstructedOptional(std::move(moveConstructedValue));
121+
ASSERT_FALSE(moveConstructedOptional->m_copied);
122+
ASSERT_TRUE(moveConstructedOptional->m_moved);
123+
124+
Aws::Crt::Optional<CopyMoveTester> moveAssignedValue = std::move(*moveConstructedOptional);
125+
ASSERT_FALSE(moveAssignedValue->m_copied);
126+
ASSERT_TRUE(moveAssignedValue->m_moved);
127+
128+
Aws::Crt::Optional<CopyMoveTester> moveAssignedOptional = std::move(moveAssignedValue);
129+
ASSERT_FALSE(moveAssignedOptional->m_copied);
130+
ASSERT_TRUE(moveAssignedOptional->m_moved);
131+
132+
return AWS_OP_SUCCESS;
133+
}
134+
135+
AWS_TEST_CASE(OptionalCopyAndMoveSemantics, s_OptionalCopyAndMoveSemantics)

0 commit comments

Comments
 (0)