[gardening][tsan] Data race in boringssl/src/crypto/bio/pair.c:244:14 in bio_write about sdk

[rmacnak-google]

Duplicate of #53136?

@dcharkes I think this is just a low-likelihood event rather than a change in TSAN. The ffi test issue was because I'm trying to expand the set of test suites that we run under TSAN.

[dcharkes]

Trying a longer range.

dart tools/bisect.dart \
-Dend=54031e2752d76a95fa7fa069bf1b41a21260b5e9 \
-Dstart=25a309a85c449d1dce414096f1a6bc10d0bd5f7f \
-Dtest_command="python3 tools/build.py --sanitizer=tsan -ax64 -mrelease runtime runtime_precompiled" \
-Dtest_command="python3 tools/test.py -n vm-tsan-linux-release-x64 standalone/io/secure_session_resume_test/3" \
-Dfailure_string="data race"

Commit range dd3fbff...25a309a.
Ensuring SDK repo in /usr/local/google/home/dacoharkes/dart-sdk/sdk/.
Ensuring failure reproduces on 25a309a.
Commit 25a309a, reproduces failure.
Ensuring failure does not reproduce on dd3fbff.
Commit dd3fbff, reproduces failure.

So this is likely due to TSAN getting smarter.

@rmacnak-google Did we get new TSAN features recently? (Also given that ed5aeab recently showed up.)

[dcharkes]

The race is between a write in a runtime entry

sdk/runtime/bin/secure_socket_filter.cc

Lines 147 to 155 in 84fed14

	void FUNCTION_NAME(SecureSocket_Handshake)(Dart_NativeArguments args) {
	Dart_Handle port = ThrowIfError(Dart_GetNativeArgument(args, 1));
	ASSERT(!Dart_IsNull(port));
	Dart_Port port_id;
	ThrowIfError(Dart_SendPortGetId(port, &port_id));
	int result = GetFilter(args)->Handshake(port_id);
	Dart_SetReturnValue(args, Dart_NewInteger(result));
	}

And a message handler:

sdk/runtime/bin/secure_socket_filter.cc

Lines 222 to 274 in 84fed14

	/**
	* Pushes data through the SSL filter, reading and writing from circular
	* buffers shared with Dart.
	*
	* The Dart _SecureFilterImpl class contains 4 ExternalByteArrays used to
	* pass encrypted and plaintext data to and from the C++ SSLFilter object.
	*
	* ProcessFilter is called with a CObject array containing the pointer to
	* the SSLFilter, encoded as an int, and the start and end positions of the
	* valid data in the four circular buffers. The function only reads from
	* the valid data area of the input buffers, and only writes to the free
	* area of the output buffers. The function returns the new start and end
	* positions in the buffers, but it only updates start for input buffers, and
	* end for output buffers. Therefore, the Dart thread can simultaneously
	* write to the free space and end pointer of input buffers, and read from
	* the data space of output buffers, and modify the start pointer.
	*
	* When ProcessFilter returns, the Dart thread is responsible for combining
	* the updated pointers from Dart and C++, to make the new valid state of
	* the circular buffer.
	*/
	CObject* SSLFilter::ProcessFilterRequest(const CObjectArray& request) {
	CObjectIntptr filter_object(request[0]);
	SSLFilter* filter = reinterpret_cast<SSLFilter*>(filter_object.Value());
	RefCntReleaseScope<SSLFilter> rs(filter);
	bool in_handshake = CObjectBool(request[1]).Value();
	int starts[SSLFilter::kNumBuffers];
	int ends[SSLFilter::kNumBuffers];
	for (int i = 0; i < SSLFilter::kNumBuffers; ++i) {
	starts[i] = CObjectInt32(request[2 * i + 2]).Value();
	ends[i] = CObjectInt32(request[2 * i + 3]).Value();
	}
	if (filter->ProcessAllBuffers(starts, ends, in_handshake)) {
	CObjectArray* result =
	new CObjectArray(CObject::NewArray(SSLFilter::kNumBuffers * 2));
	for (int i = 0; i < SSLFilter::kNumBuffers; ++i) {
	result->SetAt(2 * i, new CObjectInt32(CObject::NewInt32(starts[i])));
	result->SetAt(2 * i + 1, new CObjectInt32(CObject::NewInt32(ends[i])));
	}
	return result;
	} else {
	int32_t error_code = static_cast<int32_t>(ERR_peek_error());
	TextBuffer error_string(SecureSocketUtils::SSL_ERROR_MESSAGE_BUFFER_SIZE);
	SecureSocketUtils::FetchErrorString(filter->ssl_, &error_string);
	CObjectArray* result = new CObjectArray(CObject::NewArray(2));
	result->SetAt(0, new CObjectInt32(CObject::NewInt32(error_code)));
	result->SetAt(1,
	new CObjectString(CObject::NewString(error_string.buffer())));
	return result;
	}
	}

FYI @bkonyi

[bkonyi]

FYI @brianquinlan

[dcharkes]

@dcharkes I think this is just a low-likelihood event rather than a change in TSAN.

It reliably reproduces for me locally on various commits.

The ffi test issue was because I'm trying to expand the set of test suites that we run under TSAN.

👍

[dcharkes]

Also https://dart-ci.appspot.com/log/vm-tsan-linux-release-x64/vm-tsan-linux-release-x64/439/standalone/io/secure_session_resume_test/2

[a-siva]

Marking this as a duplicate of #53136

There is a question about whether the secure session resume API is deprecated and this test could be removed.