fix(skill): release lock before invoking execute-done callback

ExecuteSkillDone and OnTimeout previously invoked callback->OnExecuteDone
under mutex_. Since ffrt::mutex is non-recursive, a synchronous re-entry
from the remote side back into SkillExecuteManager deadlocked the event
handler thread. Even without re-entry, holding the lock across an outbound
IPC while the business process is unresponsive stalls every other
SkillExecuteManager operation.

Move the callback invocation outside the critical section: flip the record
state, run cleanup (RemoveSkillExecuteTimeoutLocked, RemoveRecord) under
the lock, then drop the lock before issuing OnExecuteDone. Collapse
OnTimeout's two-phase critical section into one to remove the TOCTOU
window.

Add skill_execute_manager_test covering happy paths, missing-record and
state-mismatch failures, duplicate seq, post-done timeout, and the two
re-entrancy scenarios that would have deadlocked pre-fix.

Co-Authored-By: Agent
Signed-off-by: RuiChen_01 <chenrui193@huawei.com>

🤖‍ AI[100%] 👌 AI Adopted[100%] 🧑 Human[0%]
Co-authored-by: claude (glm-5.2) <ai@local>

services/abilitymgr/include/skill/skill_execute_manager.h

@@ -33,6 +33,7 @@ namespace AAFwk {
 
 class SkillExecuteManager {
 DECLARE_DELAYED_SINGLETON(SkillExecuteManager)
+friend class SkillExecuteManagerTest;
 public:
     int32_t GenerateSkillWant(const AppExecFwk::SkillInfo &skillInfo, Want &want,
         int32_t userId, const std::string &requestCode, AppExecFwk::ExtensionAbilityType &targetType,

services/abilitymgr/src/skill/skill_execute_manager.cpp

@@ -186,46 +186,53 @@ int32_t SkillExecuteManager::ExecuteSkillDone(const std::string &requestCode, in
     TAG_LOGD(AAFwkTag::ABILITYMGR,
         "execute skill done, requestCode:%{public}s code:%{public}d",
         requestCode.c_str(), resultCode);
-    std::lock_guard<ffrt::mutex> lock(mutex_);
-    auto it = records_.find(requestCode);
-    if (it == records_.end()) {
-        TAG_LOGE(AAFwkTag::ABILITYMGR, "record not found, requestCode:%{public}s", requestCode.c_str());
-        return ERR_CODE_INVALID_ID;
-    }
 
-    auto record = it->second;
-    if (record->targetBundleName != callerBundleName) {
-        TAG_LOGE(AAFwkTag::ABILITYMGR,
-            "bundleName %{public}s and %{public}s mismatch",
-            callerBundleName.c_str(), record->targetBundleName.c_str());
-        return ERR_INVALID_VALUE;
-    }
-    if (record->state != SkillExecuteState::EXECUTING) {
-        TAG_LOGE(AAFwkTag::ABILITYMGR, "invalid state:%{public}d", static_cast<int>(record->state));
-        return ERR_INVALID_VALUE;
-    }
+    sptr<ISkillExecuteCallback> callback;
+    {
+        std::lock_guard<ffrt::mutex> lock(mutex_);
+        auto it = records_.find(requestCode);
+        if (it == records_.end()) {
+            TAG_LOGE(AAFwkTag::ABILITYMGR, "record not found, requestCode:%{public}s",
+                requestCode.c_str());
+            return ERR_CODE_INVALID_ID;
+        }
+
+        auto record = it->second;
+        if (record->targetBundleName != callerBundleName) {
+            TAG_LOGE(AAFwkTag::ABILITYMGR,
+                "bundleName %{public}s and %{public}s mismatch",
+                callerBundleName.c_str(), record->targetBundleName.c_str());
+            return ERR_INVALID_VALUE;
+        }
+        if (record->state != SkillExecuteState::EXECUTING) {
+            TAG_LOGE(AAFwkTag::ABILITYMGR, "invalid state:%{public}d", static_cast<int>(record->state));
+            return ERR_INVALID_VALUE;
+        }
 
 #ifdef SUPPORT_UPMS
-    if (!result.uris.empty() && !record->callerBundleName.empty()) {
-        std::vector<Uri> uriList;
-        for (const auto &uriStr : result.uris) {
-            uriList.emplace_back(uriStr);
+        if (!result.uris.empty() && !record->callerBundleName.empty()) {
+            std::vector<Uri> uriList;
+            for (const auto &uriStr : result.uris) {
+                uriList.emplace_back(uriStr);
+            }
+            auto &uriPermClient = UriPermissionManagerClient::GetInstance();
+            auto ret = uriPermClient.GrantUriPermission(
+                uriList, result.flags, record->callerBundleName, 0, record->callerTokenId);
+            if (ret != ERR_OK) {
+                TAG_LOGE(AAFwkTag::ABILITYMGR, "GrantUriPermission failed, ret:%{public}d", ret);
+            }
         }
-        auto &uriPermClient = UriPermissionManagerClient::GetInstance();
-        auto ret = uriPermClient.GrantUriPermission(
-            uriList, result.flags, record->callerBundleName, 0, record->callerTokenId);
-        if (ret != ERR_OK) {
-            TAG_LOGE(AAFwkTag::ABILITYMGR, "GrantUriPermission failed, ret:%{public}d", ret);
-        }
-    }
 #endif
 
-    RemoveSkillExecuteTimeoutLocked(record->requestCodeSeq);
-    record->state = SkillExecuteState::EXECUTE_DONE;
-    if (record->callback != nullptr) {
-        record->callback->OnExecuteDone(requestCode, resultCode, result);
+        RemoveSkillExecuteTimeoutLocked(record->requestCodeSeq);
+        record->state = SkillExecuteState::EXECUTE_DONE;
+        callback = record->callback;
+        RemoveRecord(requestCode);
+    }
+
+    if (callback != nullptr) {
+        callback->OnExecuteDone(requestCode, resultCode, result);
     }
-    RemoveRecord(requestCode);
     return ERR_OK;
 }
 
@@ -354,33 +361,38 @@ void SkillExecuteManager::RemoveSkillExecuteTimeoutLocked(uint64_t requestCodeSe
 void SkillExecuteManager::OnTimeout(int64_t requestCodeSeq)
 {
     TAG_LOGI(AAFwkTag::ABILITYMGR, "called, seq:%{public}" PRId64, requestCodeSeq);
+
+    sptr<ISkillExecuteCallback> callback;
     std::string requestCode;
     {
         std::lock_guard<ffrt::mutex> lock(mutex_);
         auto seqIt = seqToRequestCodeMap_.find(requestCodeSeq);
         if (seqIt == seqToRequestCodeMap_.end()) {
-            TAG_LOGW(AAFwkTag::ABILITYMGR, "seq not found");
+            TAG_LOGW(AAFwkTag::ABILITYMGR, "seq not found, seq:%{public}" PRId64, requestCodeSeq);
             return;
         }
         requestCode = seqIt->second;
         seqToRequestCodeMap_.erase(seqIt);
+
+        auto it = records_.find(requestCode);
+        if (it == records_.end()) {
+            return;
+        }
+        auto &record = it->second;
+        if (record->state != SkillExecuteState::EXECUTING) {
+            return;
+        }
+
+        TAG_LOGW(AAFwkTag::ABILITYMGR, "skill execute timed out, req:%{public}s", requestCode.c_str());
+        record->state = SkillExecuteState::TIMED_OUT;
+        callback = record->callback;
+        RemoveRecord(requestCode);
     }
-    std::lock_guard<ffrt::mutex> lock(mutex_);
-    auto it = records_.find(requestCode);
-    if (it == records_.end()) {
-        return;
-    }
-    auto record = it->second;
-    if (record->state != SkillExecuteState::EXECUTING) {
-        return;
-    }
-    TAG_LOGW(AAFwkTag::ABILITYMGR, "skill execute timed out, req:%{public}s", requestCode.c_str());
-    record->state = SkillExecuteState::TIMED_OUT;
-    if (record->callback != nullptr) {
+
+    if (callback != nullptr) {
         AppExecFwk::SkillExecuteResult emptyResult;
-        record->callback->OnExecuteDone(requestCode, ERR_TIMED_OUT, emptyResult);
+        callback->OnExecuteDone(requestCode, ERR_TIMED_OUT, emptyResult);
     }
-    RemoveRecord(requestCode);
 }
 
 } // namespace AAFwk

test/unittest/BUILD.gn

@@ -519,6 +519,7 @@ group("unittest") {
       "service_router_mgr_service_test:unittest",
       "skill_execute_callback_proxy_test:unittest",
       "skill_execute_callback_stub_test:unittest",
+      "skill_execute_manager_test:unittest",
       "skill_execute_param_test:unittest",
       "skill_execute_result_test:unittest",
       "services/ability_util_test:unittest",

test/unittest/skill_execute_manager_test/BUILD.gn

@@ -0,0 +1,63 @@
+# Copyright (c) 2026 Huawei Device Co., Ltd.
+# 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.
+
+import("//build/test.gni")
+import("//foundation/ability/ability_runtime/ability_runtime.gni")
+
+module_output_path = "ability_runtime/ability_runtime/skill"
+
+ohos_unittest("skill_execute_manager_test") {
+  module_out_path = module_output_path
+
+  include_dirs = [
+    "${ability_runtime_test_path}/mock/services_abilitymgr_test/libs/system_ability_mock",
+    "${ability_runtime_test_path}/unittest/skill_execute_callback_proxy_test",
+  ]
+
+  sources = [ "skill_execute_manager_test.cpp" ]
+
+  configs = [
+    "${ability_runtime_services_path}/abilitymgr:abilityms_config",
+    "${ability_runtime_test_path}/mock/services_abilitymgr_test:aafwk_mock_config",
+  ]
+
+  cflags = []
+  if (target_cpu == "arm") {
+    cflags += [ "-DBINDER_IPC_32BIT" ]
+  }
+
+  deps = [
+    "${ability_runtime_innerkits_path}/ability_manager:ability_manager",
+    "${ability_runtime_innerkits_path}/deps_wrapper:ability_deps_wrapper",
+    "${ability_runtime_services_path}/abilitymgr:abilityms",
+    "${ability_runtime_services_path}/abilitymgr:abilityms_target",
+  ]
+
+  external_deps = [
+    "ability_base:want",
+    "ability_base:zuri",
+    "bundle_framework:libappexecfwk_common",
+    "c_utils:utils",
+    "ffrt:libffrt",
+    "googletest:gmock_main",
+    "googletest:gtest_main",
+    "hilog:libhilog",
+    "ipc:ipc_core",
+    "samgr:samgr_proxy",
+  ]
+}
+
+group("unittest") {
+  testonly = true
+  deps = [ ":skill_execute_manager_test" ]
+}

test/unittest/skill_execute_manager_test/skill_execute_manager_test.cpp

@@ -0,0 +1,274 @@
+/*
+ * Copyright (c) 2026 Huawei Device Co., Ltd.
+ * 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 <gtest/gtest.h>
+
+#include <memory>
+
+#include "ability_manager_errors.h"
+#include "hilog_tag_wrapper.h"
+#include "mock_skill_execute_callback_stub.h"
+#include "singleton.h"
+#include "skill/skill_execute_manager.h"
+#include "skill/skill_execute_record.h"
+#include "skill_execute_result.h"
+
+using namespace testing::ext;
+using namespace testing;
+
+namespace OHOS {
+namespace AAFwk {
+
+namespace {
+constexpr const char *TARGET_BUNDLE = "com.example.target";
+constexpr const char *CALLER_BUNDLE = "com.example.caller";
+} // namespace
+
+class SkillExecuteManagerTest : public testing::Test {
+public:
+    static void SetUpTestCase(void) {}
+    static void TearDownTestCase(void) {}
+    void SetUp() override;
+    void TearDown() override;
+
+protected:
+    void SeedRecord(const std::string &requestCode, uint64_t seq,
+        SkillExecuteState state = SkillExecuteState::EXECUTING);
+
+    std::shared_ptr<SkillExecuteManager> mgr_;
+    sptr<MockSkillExecuteCallbackStub> callback_;
+};
+
+void SkillExecuteManagerTest::SetUp()
+{
+    mgr_ = DelayedSingleton<SkillExecuteManager>::GetInstance();
+    ASSERT_NE(mgr_, nullptr);
+    callback_ = new MockSkillExecuteCallbackStub();
+    ASSERT_NE(callback_, nullptr);
+}
+
+void SkillExecuteManagerTest::TearDown()
+{
+    {
+        std::lock_guard<ffrt::mutex> lock(mgr_->mutex_);
+        mgr_->records_.clear();
+        mgr_->seqToRequestCodeMap_.clear();
+    }
+    callback_ = nullptr;
+    mgr_ = nullptr;
+}
+
+void SkillExecuteManagerTest::SeedRecord(const std::string &requestCode, uint64_t seq,
+    SkillExecuteState state)
+{
+    auto record = std::make_shared<SkillExecuteRecord>();
+    record->requestCode = requestCode;
+    record->targetBundleName = TARGET_BUNDLE;
+    record->callerBundleName = CALLER_BUNDLE;
+    record->requestCodeSeq = seq;
+    record->state = state;
+    record->callback = callback_;
+
+    std::lock_guard<ffrt::mutex> lock(mgr_->mutex_);
+    mgr_->records_[requestCode] = record;
+    mgr_->seqToRequestCodeMap_[seq] = requestCode;
+}
+
+/**
+ * @tc.name: ExecuteSkillDone_0100
+ * @tc.desc: ExecuteSkillDone happy path: callback invoked, record removed.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, ExecuteSkillDone_0100, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req001", 1);
+    EXPECT_CALL(*callback_, OnExecuteDone(Eq("req001"), Eq(0), _)).Times(1);
+
+    AppExecFwk::SkillExecuteResult result;
+    auto ret = mgr_->ExecuteSkillDone("req001", 0, result, TARGET_BUNDLE);
+    EXPECT_EQ(ret, ERR_OK);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: ExecuteSkillDone_0200
+ * @tc.desc: ExecuteSkillDone on unknown requestCode returns ERR_CODE_INVALID_ID.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, ExecuteSkillDone_0200, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(0);
+
+    AppExecFwk::SkillExecuteResult result;
+    auto ret = mgr_->ExecuteSkillDone("nonexistent", 0, result, TARGET_BUNDLE);
+    EXPECT_EQ(ret, ERR_CODE_INVALID_ID);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: ExecuteSkillDone_0300
+ * @tc.desc: ExecuteSkillDone with bundle mismatch returns ERR_INVALID_VALUE.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, ExecuteSkillDone_0300, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req003", 3);
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(0);
+
+    AppExecFwk::SkillExecuteResult result;
+    auto ret = mgr_->ExecuteSkillDone("req003", 0, result, "com.example.wrong");
+    EXPECT_EQ(ret, ERR_INVALID_VALUE);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: ExecuteSkillDone_0400
+ * @tc.desc: ExecuteSkillDone twice: second call fails because record was removed.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, ExecuteSkillDone_0400, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req004", 4);
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(1);
+
+    AppExecFwk::SkillExecuteResult result;
+    auto ret1 = mgr_->ExecuteSkillDone("req004", 0, result, TARGET_BUNDLE);
+    EXPECT_EQ(ret1, ERR_OK);
+
+    auto ret2 = mgr_->ExecuteSkillDone("req004", 0, result, TARGET_BUNDLE);
+    EXPECT_EQ(ret2, ERR_CODE_INVALID_ID);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: ExecuteSkillDone_0500
+ * @tc.desc: ExecuteSkillDone reentrant from callback must not deadlock.
+ *          Pre-fix: callback ran under mutex_; re-entering deadlocked.
+ *          Post-fix: callback runs outside mutex_; re-entry is safe.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, ExecuteSkillDone_0500, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req005", 5);
+
+    auto mgr = mgr_;
+    EXPECT_CALL(*callback_, OnExecuteDone(Eq("req005"), _, _))
+        .Times(1)
+        .WillOnce(Invoke([mgr](const std::string &reqCode, int32_t,
+            const AppExecFwk::SkillExecuteResult &) {
+            AppExecFwk::SkillExecuteResult inner;
+            auto ret = mgr->ExecuteSkillDone(reqCode, 0, inner, TARGET_BUNDLE);
+            EXPECT_EQ(ret, ERR_CODE_INVALID_ID);
+        }));
+
+    AppExecFwk::SkillExecuteResult result;
+    auto ret = mgr_->ExecuteSkillDone("req005", 0, result, TARGET_BUNDLE);
+    EXPECT_EQ(ret, ERR_OK);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: OnTimeout_0100
+ * @tc.desc: OnTimeout on EXECUTING record flips state, fires callback once.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, OnTimeout_0100, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req101", 101);
+    EXPECT_CALL(*callback_, OnExecuteDone(Eq("req101"), Eq(ERR_TIMED_OUT), _)).Times(1);
+
+    mgr_->OnTimeout(101);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: OnTimeout_0200
+ * @tc.desc: OnTimeout with unknown seq does nothing.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, OnTimeout_0200, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(0);
+
+    mgr_->OnTimeout(999);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: OnTimeout_0300
+ * @tc.desc: OnTimeout twice on same seq: second call is a no-op.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, OnTimeout_0300, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req103", 103);
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(1);
+
+    mgr_->OnTimeout(103);
+    mgr_->OnTimeout(103);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: OnTimeout_0400
+ * @tc.desc: OnTimeout after ExecuteSkillDone: record gone, no callback.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, OnTimeout_0400, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req104", 104);
+    EXPECT_CALL(*callback_, OnExecuteDone(_, _, _)).Times(1);
+
+    AppExecFwk::SkillExecuteResult result;
+    mgr_->ExecuteSkillDone("req104", 0, result, TARGET_BUNDLE);
+    mgr_->OnTimeout(104);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+/**
+ * @tc.name: OnTimeout_0500
+ * @tc.desc: OnTimeout reentrant from callback must not deadlock.
+ *          Pre-fix: callback ran under mutex_; re-entering deadlocked.
+ *          Post-fix: callback runs outside mutex_; re-entry is safe.
+ * @tc.type: FUNC
+ */
+HWTEST_F(SkillExecuteManagerTest, OnTimeout_0500, TestSize.Level1)
+{
+    TAG_LOGI(AAFwkTag::TEST, "begin");
+    SeedRecord("req105", 105);
+
+    auto mgr = mgr_;
+    EXPECT_CALL(*callback_, OnExecuteDone(Eq("req105"), _, _))
+        .Times(1)
+        .WillOnce(Invoke([mgr](const std::string &, int32_t,
+            const AppExecFwk::SkillExecuteResult &) {
+            mgr->OnTimeout(105); // seq already erased, no-op
+        }));
+
+    mgr_->OnTimeout(105);
+    TAG_LOGI(AAFwkTag::TEST, "end");
+}
+
+} // namespace AAFwk
+} // namespace OHOS