Skip to content

Commit bb8cdd0

Browse files
bretambrosebretambrose
authored
Merge pull request #72 from awslabs/FB-AndroidStringBuf
Bombs Away
2 parents 010fb4f + b67ed92 commit bb8cdd0

File tree

13 files changed

+830
-20
lines changed

13 files changed

+830
-20
lines changed

‎AndroidSDKTesting/app/src/main/java/aws/androidsdktesting/RunSDKTests.java

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,18 @@ protected Boolean doInBackground(String... testNames)
111111
Log.i("AwsNativeSDK", "Loading common libraries ");
112112

113113
//System.loadLibrary("c");
114-
System.loadLibrary("c++_shared");
114+
try {
115+
System.loadLibrary("c++_shared");
116+
} catch (Exception e) {
117+
;
118+
}
119+
120+
try {
121+
System.loadLibrary("gnustl_shared");
122+
} catch (Exception e) {
123+
;
124+
}
125+
115126
System.loadLibrary("log");
116127
System.loadLibrary("aws-cpp-sdk-core");
117128
System.loadLibrary("testing-resources");

‎android-build/build_and_test_android.py

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ def ParseArguments():
4343
parser.add_argument("--credentials", action="store")
4444
parser.add_argument("--build", action="store")
4545
parser.add_argument("--so", action="store_true")
46+
parser.add_argument("--stl", action="store")
4647

4748
args = vars( parser.parse_args() )
4849

@@ -57,6 +58,7 @@ def ParseArguments():
5758
argMap[ "buildType" ] = args[ "build" ] or "Release"
5859
argMap[ "runTest" ] = args[ "runtest" ]
5960
argMap[ "so" ] = args[ "so" ]
61+
argMap[ "stl" ] = args[ "stl" ] or "libc++_shared"
6062

6163
return argMap
6264

@@ -169,16 +171,19 @@ def SetupJniDirectory(abi, clean):
169171
return path
170172

171173

172-
def CopyNativeLibraries(buildSharedObjects, jniDir, buildDir, abi):
173-
# TODO: fix hardcoded 13b version
174-
toolchainName = abi + "-standalone-clang-android-21-libc++_shared-13002"
175-
toolchainDir = os.path.join('toolchains', 'android', toolchainName)
174+
def CopyNativeLibraries(buildSharedObjects, jniDir, buildDir, abi, stl):
175+
baseToolchainDir = os.path.join(buildDir, 'toolchains', 'android')
176+
toolchainDirList = os.listdir(baseToolchainDir) # should only be one entry
177+
toolchainDir = os.path.join(baseToolchainDir, toolchainDirList[0])
176178

177179
platformLibDir = os.path.join(toolchainDir, "sysroot", "usr", "lib")
178180
shutil.copy(os.path.join(platformLibDir, "liblog.so"), jniDir)
179181

180182
stdLibDir = os.path.join(toolchainDir, 'arm-linux-androideabi', 'lib')
181-
shutil.copy(os.path.join(stdLibDir, "libc++_shared.so"), jniDir)
183+
if stl == 'libc++_shared':
184+
shutil.copy(os.path.join(stdLibDir, "libc++_shared.so"), jniDir)
185+
elif stl == 'gnustl_shared':
186+
shutil.copy(os.path.join(stdLibDir, "armv7-a", "libgnustl_shared.so"), jniDir) # TODO: remove armv7-a hardcoded path
182187

183188
if buildSharedObjects:
184189

@@ -199,7 +204,7 @@ def RemoveTree(dir):
199204
shutil.rmtree( dir )
200205

201206

202-
def BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildSharedObjects):
207+
def BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildSharedObjects, stl):
203208
if clean:
204209
RemoveTree(installDir)
205210
RemoveTree(buildDir)
@@ -221,6 +226,7 @@ def BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildShared
221226
"-DCUSTOM_MEMORY_MANAGEMENT=1",
222227
"-DTARGET_ARCH=ANDROID",
223228
"-DANDROID_ABI=" + abi,
229+
"-DANDROID_STL=" + stl,
224230
"-DCMAKE_BUILD_TYPE=" + buildType,
225231
"-DENABLE_UNITY_BUILD=ON",
226232
'-DTEST_CERT_PATH="/data/data/aws.' + TestLowerName + '/certs"',
@@ -235,7 +241,7 @@ def BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildShared
235241
subprocess.check_call( [ "make", "-j12", "android-unified-tests" ] )
236242

237243
os.chdir( ".." )
238-
CopyNativeLibraries(buildSharedObjects, jniDir, buildDir, abi)
244+
CopyNativeLibraries(buildSharedObjects, jniDir, buildDir, abi, stl)
239245

240246

241247
def BuildJava(clean):
@@ -400,6 +406,7 @@ def Main():
400406
noInstall = args[ "noInstall" ]
401407
buildSharedObjects = args[ "so" ]
402408
runTest = args[ "runTest" ]
409+
stl = args[ "stl" ]
403410

404411
buildDir = "_build" + buildType
405412
installDir = os.path.join( "external", abi );
@@ -410,7 +417,7 @@ def Main():
410417
jniDir = SetupJniDirectory(abi, clean)
411418

412419
if not skipBuild:
413-
BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildSharedObjects)
420+
BuildNative(abi, clean, buildDir, jniDir, installDir, buildType, buildSharedObjects, stl)
414421
BuildJava(clean)
415422

416423
if not runTest:

‎aws-cpp-sdk-core-tests/utils/memory/SimpleStringStreamTests.cpp

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/*
2+
* Copyright 2010-2016 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+
16+
#include <aws/external/gtest.h>
17+
#include <aws/core/utils/memory/stl/SimpleStringStream.h>
18+
19+
TEST(SimpleStringStreamTest, DefaultConstructor)
20+
{
21+
Aws::SimpleStringStream ss;
22+
23+
ASSERT_TRUE(ss.str().size() == 0);
24+
}
25+
26+
static const char* SIMPLE_STRING = "A Simple String";
27+
28+
TEST(SimpleStringStreamTest, StringConstructor)
29+
{
30+
Aws::SimpleStringStream ss(SIMPLE_STRING);
31+
32+
ASSERT_STREQ(ss.str().c_str(), SIMPLE_STRING);
33+
}
34+
35+
TEST(SimpleStringStreamTest, BasicOutput)
36+
{
37+
Aws::SimpleStringStream ss;
38+
39+
ss << SIMPLE_STRING;
40+
41+
ASSERT_STREQ(ss.str().c_str(), SIMPLE_STRING);
42+
}
43+
44+
TEST(SimpleStringStreamTest, MultipleOutput)
45+
{
46+
Aws::SimpleStringStream ss;
47+
48+
ss << "A string " << Aws::String("\"Howdy\"");
49+
ss << ", a number " << 75;
50+
ss << ", and a boolean " << std::boolalpha << true;
51+
ss << " walk into a bar";
52+
53+
ASSERT_STREQ(ss.str().c_str(), "A string \"Howdy\", a number 75, and a boolean true walk into a bar");
54+
}
55+
56+
TEST(SimpleStringStreamTest, MultipleInput)
57+
{
58+
Aws::SimpleStringStream ss("523 47.0 true");
59+
60+
uint32_t number = 0;
61+
ss >> number;
62+
ASSERT_TRUE(number == 523);
63+
64+
double fp = 0.0;
65+
ss >> fp;
66+
ASSERT_DOUBLE_EQ(fp, 47.0);
67+
68+
bool value = false;
69+
ss >> std::boolalpha >> value;
70+
ASSERT_TRUE(value);
71+
}
72+

‎aws-cpp-sdk-core-tests/utils/stream/SimpleStreamBufTest.cpp

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* Copyright 2010-2016 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+
16+
#include <aws/external/gtest.h>
17+
#include <aws/core/utils/Array.h>
18+
#include <aws/core/utils/stream/SimpleStreamBuf.h>
19+
#include <aws/core/utils/memory/stl/AWSString.h>
20+
#include <aws/core/utils/memory/stl/AWSStreamFwd.h>
21+
22+
using namespace Aws::Utils;
23+
using namespace Aws::Utils::Stream;
24+
25+
// these are all just variants of some of the PreallocatedStreamBuf tests
26+
const char bufferStr[] = "This is an internal buffer.";
27+
const char replacementBuf[] = "Boom, I ruined your st";
28+
const char concatStr[] = "This Boom, I ruined your st";
29+
30+
31+
//Write to an empty buffer via a stream interface, and make sure the buffer
32+
//contains the data.
33+
TEST(SimpleStreamBufTest, TestStreamWriteToPrefilledBuffer)
34+
{
35+
SimpleStreamBuf streamBuf;
36+
Aws::IOStream ioStream(&streamBuf);
37+
ioStream.write(bufferStr, sizeof(bufferStr));
38+
39+
ASSERT_STREQ(bufferStr, streamBuf.str().c_str());
40+
}
41+
42+
//test read seeking from the beginning
43+
TEST(SimpleStreamBufTest, TestStreamReadSeekBeg)
44+
{
45+
SimpleStreamBuf streamBuf;
46+
Aws::IOStream ioStream(&streamBuf);
47+
ioStream.write(bufferStr, sizeof(bufferStr));
48+
49+
ioStream.seekg(5, std::ios_base::beg);
50+
Array<uint8_t> readBuf(strlen(bufferStr) + 1 - 5);
51+
ioStream.read((char*)readBuf.GetUnderlyingData(), readBuf.GetLength());
52+
ASSERT_EQ(sizeof(bufferStr) - 5, static_cast<size_t>(ioStream.gcount()));
53+
ASSERT_STREQ(bufferStr + 5, (const char*)readBuf.GetUnderlyingData());
54+
}
55+
56+
//test read seeking from current pos.
57+
TEST(SimpleStreamBufTest, TestStreamReadSeekCur)
58+
{
59+
SimpleStreamBuf streamBuf;
60+
Aws::IOStream ioStream(&streamBuf);
61+
ioStream.write(bufferStr, sizeof(bufferStr));
62+
63+
ioStream.seekg(5, std::ios_base::cur);
64+
Array<uint8_t> readBuf(sizeof(bufferStr) - 5);
65+
ioStream.read((char*)readBuf.GetUnderlyingData(), readBuf.GetLength());
66+
ASSERT_EQ(sizeof(bufferStr) - 5, static_cast<size_t>(ioStream.gcount()));
67+
ASSERT_STREQ(bufferStr + 5, (const char*)readBuf.GetUnderlyingData());
68+
}
69+
70+
//test read seeking from the end.
71+
TEST(SimpleStreamBufTest, TestStreamReadSeekEnd)
72+
{
73+
SimpleStreamBuf streamBuf;
74+
Aws::IOStream ioStream(&streamBuf);
75+
ioStream.write(bufferStr, sizeof(bufferStr));
76+
77+
auto seekPos = sizeof(bufferStr) - 5;
78+
ioStream.seekg(seekPos, std::ios_base::end);
79+
Array<uint8_t> readBuf(sizeof(bufferStr) - 5);
80+
ioStream.read((char*)readBuf.GetUnderlyingData(), readBuf.GetLength());
81+
ASSERT_EQ(sizeof(bufferStr) - 5, static_cast<size_t>(ioStream.gcount()));
82+
ASSERT_STREQ(bufferStr + 5, (const char*)readBuf.GetUnderlyingData());
83+
}
84+
85+
//test write seeking from the beginning.
86+
TEST(SimpleStreamBufTest, TestStreamWriteSeekBeg)
87+
{
88+
SimpleStreamBuf streamBuf;
89+
Aws::IOStream ioStream(&streamBuf);
90+
ioStream.write(bufferStr, sizeof(bufferStr));
91+
92+
ioStream.seekp(5, std::ios_base::beg);
93+
ioStream.write(replacementBuf, sizeof(replacementBuf));
94+
95+
ASSERT_STREQ(concatStr, streamBuf.str().c_str());
96+
}
97+
98+
//test write seeking from the end.
99+
TEST(SimpleStreamBufTest, TestStreamWriteSeekEnd)
100+
{
101+
SimpleStreamBuf streamBuf;
102+
Aws::IOStream ioStream(&streamBuf);
103+
ioStream.write(bufferStr, sizeof(bufferStr));
104+
105+
auto seekPos = strlen(bufferStr) + 1 - 5;
106+
ioStream.seekp(seekPos, std::ios_base::end);
107+
ioStream.write(replacementBuf, sizeof(replacementBuf));
108+
109+
ASSERT_STREQ(concatStr, streamBuf.str().c_str());
110+
}
111+
112+
TEST(SimpleStreamBufTest, TestZeroLengthSeekFromEnd)
113+
{
114+
SimpleStreamBuf streamBuf;
115+
Aws::IOStream ioStream(&streamBuf);
116+
ioStream.write(bufferStr, sizeof(bufferStr));
117+
118+
ioStream.seekg(0, std::ios_base::end);
119+
ASSERT_FALSE(ioStream.eof());
120+
121+
// attempting to read a character should fail and hit eof since we're one position after
122+
// the last character
123+
char ch = 0;
124+
ioStream.get(ch);
125+
// could check ch == 0 but I don't think the standard guarantees that
126+
ASSERT_TRUE(ioStream.eof());
127+
}
128+
129+
TEST(SimpleStreamBufTest, SetStr)
130+
{
131+
SimpleStreamBuf streamBuf;
132+
streamBuf.str(bufferStr);
133+
134+
ASSERT_STREQ(bufferStr, streamBuf.str().c_str());
135+
}
136+
137+
TEST(SimpleStreamBufTest, StringConstructor)
138+
{
139+
SimpleStreamBuf streamBuf(bufferStr);
140+
ASSERT_STREQ(bufferStr, streamBuf.str().c_str());
141+
}
142+

‎aws-cpp-sdk-core/CMakeLists.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ file(GLOB UTILS_THREADING_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/threa
7272
file(GLOB UTILS_XML_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/xml/*.cpp")
7373
file(GLOB UTILS_LOGGING_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/logging/*.cpp")
7474
file(GLOB UTILS_MEMORY_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/memory/*.cpp")
75+
file(GLOB UTILS_MEMORY_STL_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/memory/stl/*.cpp")
7576
file(GLOB UTILS_STREAM_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/stream/*.cpp")
7677
file(GLOB UTILS_CRYPTO_FACTORY_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/source/utils/crypto/factory/*.cpp")
7778

@@ -121,6 +122,7 @@ file(GLOB AWS_NATIVE_SDK_COMMON_SRC
121122
${UTILS_STREAM_SOURCE}
122123
${UTILS_LOGGING_SOURCE}
123124
${UTILS_MEMORY_SOURCE}
125+
${UTILS_MEMORY_STL_SOURCE}
124126
${UTILS_CRYPTO_OPENSSL_SOURCE}
125127
${UTILS_CRYPTO_COMMONCRYPTO_SOURCE}
126128
)
@@ -286,6 +288,7 @@ if(MSVC)
286288
source_group("Source Files\\utils\\stream" FILES ${UTILS_STREAM_SOURCE})
287289
source_group("Source Files\\utils\\logging" FILES ${UTILS_LOGGING_SOURCE})
288290
source_group("Source Files\\utils\\memory" FILES ${UTILS_MEMORY_SOURCE})
291+
source_group("Source Files\\utils\\memory\\stl" FILES ${UTILS_MEMORY_STL_SOURCE})
289292

290293
# http client conditional source
291294
if(ENABLE_CURL_CLIENT)

‎aws-cpp-sdk-core/include/aws/core/utils/memory/stl/AWSString.h

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,16 @@
2525
namespace Aws
2626
{
2727

28-
#if _GLIBCXX_FULLY_DYNAMIC_STRING == 0 && defined(__ANDROID__)
28+
#if defined(_GLIBCXX_FULLY_DYNAMIC_STRING) && _GLIBCXX_FULLY_DYNAMIC_STRING == 0 && defined(__ANDROID__)
2929

3030
/*
31-
using std::string with shared libraries is broken on android due to the platform-level decision to set _GLIBCXX_FULLY_DYNAMIC_STRING to 0
31+
using std::string with shared libraries is broken on android when using gnustl
32+
due to the platform-level decision to set _GLIBCXX_FULLY_DYNAMIC_STRING to 0
3233
3334
The problem:
3435
35-
(1) gnustl is the only usable c++11-compliant c++ standard library on Android; it is our only choice
36-
(2) _GLIBCXX_FULLY_DYNAMIC_STRING is set to 0 in Android's c++config.h for gnustl
37-
(3) The optimization that this enables is completely broken if using shared libraries and there is no way to opt out of using it.
36+
(1) _GLIBCXX_FULLY_DYNAMIC_STRING is set to 0 in Android's c++config.h for gnustl
37+
(2) The optimization that this enables is completely broken if using shared libraries and there is no way to opt out of using it.
3838
An optimization that uses a comparison to a static memory address is death for shared libraries.
3939
4040
Supposing you have a shared library B that depends on another shared library A. There are a variety of inocuous scenarios where you end up crashing
@@ -50,18 +50,28 @@ Lessons (with the empty string optimization forced on you):
5050
(1) You can't move std::strings across shared libraries (as a part of another class, Outcome in our case)
5151
(2) If you default initialize a std::string member variable, you can't have a mismatched constructor/destructor pair such that one is in a cpp file and the other
5252
is missing/implicit or defined in the header file
53-
54-
After much trouble, we have the following ghetto solution that has stopped all of the crashes so far without having to scour our entire codebase for every Lesson violation above:
53+
54+
Solutions:
55+
56+
Use libc++ rather than gnustl
57+
58+
For those who must use gnustl, we have provided a working solution by cobbling together a set of hacks:
5559
5660
We prevent the empty string optimization from ever being run on our strings by:
5761
(1) Make Aws::Allocator always fail equality checks with itself; this check is part of the empty string optimization in several std::basic_string constructors
5862
(2) All other cases are prevented by turning Aws::String into a subclass whose default constructor and move operations go to baseclass versions which will not
5963
perform the empty string optimization
6064
65+
Those changes prevent crashes, but lead to very poor performance when using a string stream; every character added will result in multiple copies of the entire
66+
string (ie, quadratic).
6167
62-
This does not prevent problems with Aws::StringBuf and Aws::StringStream. We do not appear to be violating any of the lessons with our usage of them though.
68+
To fix the performance problems, we have put together a set of replacement classes, SimpleStreamBuf and SimpleStringStream, that
69+
replace std::stringstream and std::stringbuf in SDK code. These replacements use raw buffers rather than strings in order to
70+
avoid the performance issues.
6371
72+
This solution is only enabled if using gnustl on Android. In all other situations, normal STL types are used.
6473
*/
74+
6575
using AndroidBasicString = std::basic_string< char, std::char_traits< char >, Aws::Allocator< char > >;
6676

6777
class String : public AndroidBasicString

0 commit comments

Comments
 (0)