diff --git a/common/cpp/build/tests/Makefile b/common/cpp/build/tests/Makefile index b907bfbf7..1af7e06fc 100644 --- a/common/cpp/build/tests/Makefile +++ b/common/cpp/build/tests/Makefile @@ -26,6 +26,7 @@ SOURCES_PATH := $(ROOT_SOURCES_PATH)/$(ROOT_SOURCES_SUBDIR) SOURCES := \ $(SOURCES_PATH)/formatting_unittest.cc \ $(SOURCES_PATH)/ipc_emulation_unittest.cc \ + $(SOURCES_PATH)/join_string_unittest.cc \ $(SOURCES_PATH)/logging/hex_dumping_unittest.cc \ $(SOURCES_PATH)/logging/logging_unittest.cc \ $(SOURCES_PATH)/messaging/typed_message_router_unittest.cc \ diff --git a/common/cpp/src/google_smart_card_common/join_string.h b/common/cpp/src/google_smart_card_common/join_string.h new file mode 100644 index 000000000..edc147b7b --- /dev/null +++ b/common/cpp/src/google_smart_card_common/join_string.h @@ -0,0 +1,49 @@ +// Copyright 2022 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_ +#define GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_ + +#include + +namespace google_smart_card { + +template +std::string JoinStrings(const Container& container, + const std::string& separator) { + if (container.empty()) { + return ""; + } + + size_t need_size = separator.size() * (container.size() - 1); + for (const auto& item : container) { + need_size += item.length(); + } + + std::string joined; + joined.reserve(need_size); + bool is_first = true; + for (const auto& item : container) { + if (!is_first) { + joined += separator; + } + is_first = false; + joined += item; + } + return joined; +} + +} // namespace google_smart_card + +#endif // GOOGLE_SMART_CARD_COMMON_JOIN_STRING_H_ diff --git a/common/cpp/src/google_smart_card_common/join_string_unittest.cc b/common/cpp/src/google_smart_card_common/join_string_unittest.cc new file mode 100644 index 000000000..eb48369a8 --- /dev/null +++ b/common/cpp/src/google_smart_card_common/join_string_unittest.cc @@ -0,0 +1,62 @@ +// Copyright 2022 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include +#include +#include + +#include + +namespace google_smart_card { +namespace { + +TEST(JoinString, EmptyVector) { + std::vector vec; + EXPECT_EQ(JoinStrings(vec, /*separator=*/","), ""); +} + +TEST(JoinString, OneItemVector) { + std::vector vec = {"foo"}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo"); +} + +TEST(JoinString, TwoItemsVector) { + std::vector vec = {"foo", "bar"}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo,bar"); +} + +TEST(JoinString, TwoItemsSet) { + std::set vec = {"a", "b"}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/" - "), "a - b"); +} + +TEST(JoinString, EmptySeparator) { + std::vector vec = {"foo", "bar", "foo"}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/""), "foobarfoo"); +} + +TEST(JoinString, EmptyItem) { + std::vector vec = {"foo", "", "bar"}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/","), "foo,,bar"); +} + +TEST(JoinString, EmptyItemsOnly) { + std::vector vec = {"", "", ""}; + EXPECT_EQ(JoinStrings(vec, /*separator=*/","), ",,"); +} + +} // namespace +} // namespace google_smart_card diff --git a/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.cc b/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.cc index d996aaffd..54e349eae 100644 --- a/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.cc +++ b/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.cc @@ -103,6 +103,17 @@ bool PcscLiteClientHandlesRegistry::ContainsHandle( return handle_to_context_map_.count(s_card_handle); } +SCARDCONTEXT PcscLiteClientHandlesRegistry::FindContextByHandle( + SCARDHANDLE s_card_handle) const { + const std::unique_lock lock(mutex_); + + auto iter = handle_to_context_map_.find(s_card_handle); + if (iter == handle_to_context_map_.end()) { + return 0; + } + return iter->second; +} + void PcscLiteClientHandlesRegistry::AddHandle(SCARDCONTEXT s_card_context, SCARDHANDLE s_card_handle) { const std::unique_lock lock(mutex_); diff --git a/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.h b/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.h index 6cca8c999..be99e8cce 100644 --- a/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.h +++ b/third_party/pcsc-lite/naclport/server_clients_management/src/client_handles_registry.h @@ -62,6 +62,8 @@ class PcscLiteClientHandlesRegistry final { std::vector PopAllContexts(); bool ContainsHandle(SCARDHANDLE s_card_handle) const; + // Returns the context that this handle refers to, or zero if none found. + SCARDCONTEXT FindContextByHandle(SCARDHANDLE s_card_handle) const; // Adds the handle to the data structure. CHECKs that it wasn't already // present. void AddHandle(SCARDCONTEXT s_card_context, SCARDHANDLE s_card_handle); diff --git a/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.cc b/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.cc index 36c2db39a..af1a623d5 100644 --- a/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.cc +++ b/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.cc @@ -32,11 +32,14 @@ #include #include #include +#include +#include #include #include #include #include +#include #include #include #include @@ -215,6 +218,55 @@ void PcscLiteClientRequestProcessor::AsyncProcessRequest( .detach(); } +PcscLiteClientRequestProcessor::ScopedConcurrencyGuard::ScopedConcurrencyGuard( + const std::string& function_name, + SCARDCONTEXT s_card_context, + PcscLiteClientRequestProcessor& owner) + : function_name_(function_name), + s_card_context_(s_card_context), + owner_(owner) { + if (!s_card_context_) { + return; + } + + optional concurrent_functions_dump; + { + const std::unique_lock lock(owner_.mutex_); + std::multiset& running_functions = + owner_.context_to_running_functions_[s_card_context_]; + if (!running_functions.empty()) { + concurrent_functions_dump = + JoinStrings(running_functions, /*separator=*/", "); + } + running_functions.insert(function_name_); + } + + if (concurrent_functions_dump) { + GOOGLE_SMART_CARD_LOG_WARNING + << owner_.logging_prefix_ + << "Client violates threading: concurrent calls of " << function_name_ + << ", " << *concurrent_functions_dump + << " against the same SCARDCONTEXT. Future releases of Smart Card " + "Connector will forbid this: every call referring to some " + "SCARDCONTEXT must come after the previous one completed."; + } +} + +PcscLiteClientRequestProcessor::ScopedConcurrencyGuard:: + ~ScopedConcurrencyGuard() { + if (!s_card_context_) { + return; + } + const std::unique_lock lock(owner_.mutex_); + auto iter = owner_.context_to_running_functions_.find(s_card_context_); + GOOGLE_SMART_CARD_CHECK(iter != owner_.context_to_running_functions_.end()); + std::multiset& running_functions = iter->second; + running_functions.erase(running_functions.find(function_name_)); + if (running_functions.empty()) { + owner_.context_to_running_functions_.erase(iter); + } +} + void PcscLiteClientRequestProcessor::BuildHandlerMap() { AddHandlerToHandlerMap( "pcsc_lite_version_number", @@ -378,6 +430,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardReleaseContext( status_log_severity_); tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardReleaseContext", + s_card_context, *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) @@ -413,6 +467,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardConnect( tracer.AddPassedArg("dwPreferredProtocols", DebugDumpSCardProtocols(preferred_protocols)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardConnect", s_card_context, + *this); SCARDHANDLE s_card_handle; DWORD active_protocol; @@ -538,6 +594,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardReconnect( tracer.AddPassedArg("dwInitialization", DebugDumpSCardDisposition(initialization_action)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardReconnect", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -575,6 +634,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardDisconnect( tracer.AddPassedArg("dwDisposition", DebugDumpSCardDisposition(disposition_action)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardDisconnect", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = DisconnectCard(s_card_handle, disposition_action); @@ -607,6 +669,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardBeginTransaction( status_log_severity_); tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardBeginTransaction", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -635,6 +700,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardEndTransaction( tracer.AddPassedArg("dwDisposition", DebugDumpSCardDisposition(disposition_action)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardEndTransaction", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -660,6 +728,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardStatus( status_log_severity_); tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardStatus", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -735,6 +806,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardGetStatusChange( : &pcsc_lite_reader_states[0], pcsc_lite_reader_states.size())); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardGetStatusChange", + s_card_context, *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) @@ -784,6 +857,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardControl( tracer.AddPassedArg("bSendBuffer", "<" + DebugDumpSCardBufferContents(data_to_send) + ">"); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardControl", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -825,6 +901,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardGetAttrib( tracer.AddPassedArg("hCard", DebugDumpSCardHandle(s_card_handle)); tracer.AddPassedArg("dwAttrId", DebugDumpSCardAttributeId(attribute_id)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardGetAttrib", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -867,6 +946,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardSetAttrib( tracer.AddPassedArg("dwAttrId", DebugDumpSCardAttributeId(attribute_id)); tracer.AddPassedArg("pbAttr", "<" + HexDumpBytes(attribute) + ">"); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardSetAttrib", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -916,6 +998,9 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardTransmit( scard_response_protocol_information)); } tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard( + "SCardTransmit", + s_card_handles_registry_.FindContextByHandle(s_card_handle), *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsHandle(s_card_handle)) @@ -984,6 +1069,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardListReaders( tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context)); tracer.AddPassedArg("mszGroups", Value::kNullTypeTitle); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardListReaders", s_card_context, + *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) @@ -1020,6 +1107,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardListReaderGroups( status_log_severity_); tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardListReaderGroups", + s_card_context, *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) @@ -1058,6 +1147,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardCancel( status_log_severity_); tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context)); tracer.LogEntrance(); + // Note there's no `ScopedConcurrencyGuard`, since PC/SC API allows calling + // `SCardCancel()` from different threads. LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) @@ -1083,6 +1174,8 @@ GenericRequestResult PcscLiteClientRequestProcessor::SCardIsValidContext( status_log_severity_); tracer.AddPassedArg("hContext", DebugDumpSCardContext(s_card_context)); tracer.LogEntrance(); + ScopedConcurrencyGuard concurrency_guard("SCardIsValidContext", + s_card_context, *this); LONG return_code = SCARD_S_SUCCESS; if (!s_card_handles_registry_.ContainsContext(s_card_context)) diff --git a/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.h b/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.h index b1d564d38..60efb75e8 100644 --- a/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.h +++ b/third_party/pcsc-lite/naclport/server_clients_management/src/client_request_processor.h @@ -31,6 +31,8 @@ #include #include #include +#include +#include #include #include #include @@ -143,6 +145,28 @@ class PcscLiteClientRequestProcessor final std::function arguments)>; using HandlerMap = std::unordered_map; + // Helper class for making updates to `context_to_running_functions_`. On + // construction, adds the given function name and logs a warning if misuse + // detected. On destruction, undoes the change. + class ScopedConcurrencyGuard final { + public: + ScopedConcurrencyGuard(const std::string& function_name, + SCARDCONTEXT s_card_context, + PcscLiteClientRequestProcessor& owner); + + ScopedConcurrencyGuard(const ScopedConcurrencyGuard&) = delete; + ScopedConcurrencyGuard& operator=(const ScopedConcurrencyGuard&) = delete; + + ~ScopedConcurrencyGuard(); + + private: + const std::string function_name_; + const SCARDCONTEXT s_card_context_; + PcscLiteClientRequestProcessor& owner_; + }; + + friend class ScopedConcurrencyGuard; + void BuildHandlerMap(); template @@ -238,6 +262,10 @@ class PcscLiteClientRequestProcessor final // used to implement the client isolation: one client shouldn't be able to use // contexts/handles belonging to the other one. PcscLiteClientHandlesRegistry s_card_handles_registry_; + + mutable std::mutex mutex_; + std::unordered_map> + context_to_running_functions_; }; } // namespace google_smart_card