Skip to content

Commit 7ab34b0

Browse files
authored
Bugfix Database Large String Crash on Set on Desktop (#344)
A patch to the uWebsockets implementation to avoid a crash when SSL_write returns SSL_ERROR_WANT_WRITE. The previous code would delegate this error state to a non existent async handler. This new implementation re-attempts the write operation instead. Includes integration test changes to upload and download a 1mb string. I hand-tested larger than this (10mb) but chose a reduced size to run regularly in our CI due to RTDB usage limits combined with our expanded test matrix potentially running this numerous times in short order. This implementation uses a python script to patch uWebSockets/src/Socket.h during the cmake configuration process.
1 parent 9ec22a9 commit 7ab34b0

File tree

4 files changed

+180
-2
lines changed

4 files changed

+180
-2
lines changed

cmake/external_rules.cmake

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,16 @@ function(download_external_sources)
117117
file(INSTALL "${PROJECT_SOURCE_DIR}/cmake/external/leveldb.cmake"
118118
DESTINATION "${PROJECT_BINARY_DIR}/external/src/firestore/cmake/external")
119119
endif()
120+
121+
execute_process(
122+
COMMAND "python" "${PROJECT_SOURCE_DIR}/scripts/patch_websockets.py"
123+
"-file" "${PROJECT_BINARY_DIR}/external/src/uWebSockets/src/Socket.h"
124+
"-cmakefile" "${PROJECT_SOURCE_DIR}/cmake/external/uWebSockets.cmake"
125+
RESULT_VARIABLE STATUS)
126+
if( STATUS AND NOT STATUS EQUAL 0 )
127+
message(FATAL_ERROR "FAILED to patch uWebsockets/src/Socket.h")
128+
message(FATAL_ERROR "see cmake/external_rules.cmake.")
129+
endif()
120130
endif()
121131
endfunction()
122132

database/integration_test/src/integration_test.cc

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ using testing::PrintToString;
5959
using testing::UnorderedElementsAre;
6060

6161
const char kIntegrationTestRootPath[] = "integration_test_data";
62+
const size_t kLargeWriteStringLength = 1024 * 1024; // 1 Megabyte.
6263

6364
// Returns true if the given given timestamp is within 15 minutes of the
6465
// expected timestamp. The value compared against must be a Variant of type
@@ -365,7 +366,6 @@ firebase::database::DatabaseReference FirebaseDatabaseTest::CreateWorkingPath(
365366
}
366367

367368
// Test cases below.
368-
369369
TEST_F(FirebaseDatabaseTest, TestInitializeAndTerminate) {
370370
// Already tested via SetUp() and TearDown().
371371
}
@@ -503,6 +503,32 @@ class ExpectValueListener : public firebase::database::ValueListener {
503503
bool got_expected_value_;
504504
};
505505

506+
TEST_F(FirebaseDatabaseTest, TestLargeWrite) {
507+
const char* test_name = test_info_->name();
508+
SignIn();
509+
firebase::database::DatabaseReference ref = CreateWorkingPath();
510+
511+
LogDebug("Setting value.");
512+
std::string large_string;
513+
large_string.reserve(kLargeWriteStringLength);
514+
for(uint32_t i = 0; i < kLargeWriteStringLength; i++ ) {
515+
large_string.push_back('1');
516+
}
517+
518+
// Setup a listener to ensure the value changes properly.
519+
ExpectValueListener listener(large_string);
520+
ref.Child(test_name).Child("LargeString").AddValueListener(&listener);
521+
522+
// Set the value.
523+
firebase::Future<void> f1 =
524+
ref.Child(test_name).Child("LargeString").SetValue(std::string(large_string));
525+
WaitForCompletion(f1, "SetLargeString");
526+
527+
LogDebug("Listening for value to change as expected");
528+
ASSERT_TRUE(listener.WaitForExpectedValue());
529+
ref.Child(test_name).Child("LargeString").RemoveValueListener(&listener);
530+
}
531+
506532
TEST_F(FirebaseDatabaseTest, TestReadingFromPersistanceWhileOffline) {
507533
const char* test_name = test_info_->name();
508534

@@ -801,7 +827,6 @@ class LoggingValueListener : public firebase::database::ValueListener {
801827

802828
TEST_F(FirebaseDatabaseTest, TestAddAndRemoveListenerRace) {
803829
SKIP_TEST_ON_MOBILE;
804-
805830
const char* test_name = test_info_->name();
806831

807832
SignIn();

release_build_files/readme.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,8 @@ code.
579579
- Database: Fixed a potential crash that can occur as a result of a race
580580
condidtion when adding, removing and deleting `ValueListener`s or
581581
`ChildListener`s rapidly.
582+
- Database: Fixed a crash when setting large values on Windows and Mac
583+
systems ([#517](https://github.com/firebase/quickstart-unity/issues/517)).
582584

583585
### 7.1.1
584586
- Changes

scripts/patch_websockets.py

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Copyright 2021 Google LLC
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
"""Patches uWebsockets Socket.h to fix a crash sending large
16+
payloads with ssl encoding.
17+
"""
18+
from absl import app
19+
from absl import flags
20+
21+
FLAGS = flags.FLAGS
22+
23+
flags.DEFINE_string("file", None, "Sockets.h file to patch.")
24+
flags.DEFINE_string("cmakefile", None, "cmake file which downloaded uWebSockets")
25+
26+
# The line of Socket.h that we intend to start overwriting, and the
27+
# number of lines to overwite.
28+
# See: https://github.com/uNetworking/uWebSockets/blob/e402f022a43d283c4fb7c809332951c3108d6906/src/Socket.h#L309
29+
REPLACEMENT_START_LINE = 309
30+
31+
# The code which fixes the crash which will replace the existing
32+
# error handling implementation with one that retries the writes
33+
# when the SSL encoding buffer is full.
34+
REPLACEMENT_CODE = [
35+
" /* BEG Patched by firebase-cpp-sdk scripts/patch_websockets.py */\n",
36+
" bool continue_loop = true;\n",
37+
" do {\n",
38+
" sent = SSL_write(ssl, message->data, message->length);\n",
39+
" if (sent == (ssize_t) message->length) {\n",
40+
" wasTransferred = false;\n",
41+
" return true;\n",
42+
" } else if (sent < 0) {\n",
43+
" switch (SSL_get_error(ssl, sent)) {\n",
44+
" case SSL_ERROR_WANT_READ:\n",
45+
" continue_loop = false;\n",
46+
" break;\n",
47+
" case SSL_ERROR_WANT_WRITE:\n",
48+
" break;\n",
49+
" default:\n",
50+
" return false;\n",
51+
" }\n",
52+
" }\n",
53+
" } while (continue_loop);\n",
54+
" /* END Patched by firebase-cpp-sdk scripts/patch_websockets.py */\n"]
55+
56+
# Lines of the original implementation we are patching. Used as a saftey
57+
# check to ensure that we're patching this specific block of code.
58+
CODE_TO_REPLACE = [
59+
" sent = SSL_write(ssl, message->data, message->length);\n",
60+
" if (sent == (ssize_t) message->length) {\n",
61+
" wasTransferred = false;\n",
62+
" return true;\n",
63+
" } else if (sent < 0) {\n",
64+
" switch (SSL_get_error(ssl, sent)) {\n",
65+
" case SSL_ERROR_WANT_READ:\n",
66+
" break;\n",
67+
" case SSL_ERROR_WANT_WRITE:\n",
68+
" if ((getPoll() & UV_WRITABLE) == 0) {\n",
69+
" setPoll(getPoll() | UV_WRITABLE);\n",
70+
" changePoll(this);\n",
71+
" }\n",
72+
" break;\n",
73+
" default:\n",
74+
" return false;\n",
75+
" }\n",
76+
" }\n"]
77+
78+
# Confirms that the version of uWebsockets that cmake checked-out
79+
# is the same version this patch was designed to augment.
80+
UWEBSOCKETS_COMMIT = "4d94401b9c98346f9afd838556fdc7dce30561eb"
81+
VERSION_MISMATCH_ERROR = \
82+
"\n\n ERROR patch_websockets.py: patching wrong uWebsockets Version!\n\n"
83+
84+
def main(argv):
85+
"""Patches uWebSockets' Socket.h file to fix a crash when attempting
86+
large writes.
87+
88+
This code simply replaces one block of code (lines 309-326) with our
89+
custom code defined in REPLACE_BLOCK_CODE_LINES above.
90+
"""
91+
92+
with open(FLAGS.cmakefile,'r') as cmake_file:
93+
if UWEBSOCKETS_COMMIT not in cmake_file.read():
94+
raise Exception(VERSION_MISMATCH_ERROR)
95+
96+
with open(FLAGS.file,'r') as sockets_file:
97+
lines = sockets_file.readlines()
98+
sockets_file.close()
99+
100+
# Check if the file is already patched.
101+
if set(REPLACEMENT_CODE).issubset(set(lines)):
102+
print("File has already been patched. Exiting.")
103+
return
104+
105+
with open(FLAGS.file,'w') as sockets_file:
106+
overwrite_mode = False
107+
line_number = 1
108+
109+
for line in lines:
110+
# Check to see if we should start overwrite mode
111+
if( line_number == REPLACEMENT_START_LINE ):
112+
overwrite_mode = True;
113+
114+
if not overwrite_mode:
115+
# Copy the existing line from the source to the destination.
116+
sockets_file.write(line)
117+
line_number+=1
118+
else:
119+
# Overwrite mode. Do not copy source from original file to new file.
120+
# Verify each line to ensure it's one that we're intending to skip.
121+
line_to_replace = CODE_TO_REPLACE.pop(0)
122+
if line != line_to_replace:
123+
print("Line number: ", line_number)
124+
print("Unexpeced Line: ", line)
125+
print("Expected Line: ", line_to_replace)
126+
print
127+
raise Exception(VERSION_MISMATCH_ERROR)
128+
line_number+=1
129+
# Check if we've skipped the entire block. If so then we're clear
130+
# to write the patch to the new file.
131+
if len(CODE_TO_REPLACE) == 0:
132+
overwrite_mode = False
133+
sockets_file.writelines(REPLACEMENT_CODE)
134+
continue
135+
136+
sockets_file.close()
137+
138+
if __name__ == "__main__":
139+
flags.mark_flag_as_required("file")
140+
flags.mark_flag_as_required("cmakefile")
141+
app.run(main)

0 commit comments

Comments
 (0)