From 42be0057a68f30b439ccb52159fa756e82b61c98 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sat, 24 Jun 2023 19:49:02 +0200
Subject: [PATCH] map: fail if key is currently executing

If during execution of a mapping, that same mapping is replaced,
there is undefined behavior because we destroy a mapping that we are
still iterating over.

I have been using this mapping inside my kakrc to re-source the kakrc.

	map global user s %{:source "%val{config}/kakrc"<ret>} -docstring 'source "%val{config}/kakrc"'

Now <space>s happens to not trigger undefined behavior because the
mapping stays the same.

However it triggers an assertion added by Commit e49c0fb04 (unmap:
fail if the mapping is currently executing, 2023-05-14), specifically
the destructor of ScopedSetBool that guards mapping execution.

Fix these by banning map of a key that is executing, just like we
did for unmap.

Alternative solution: we could allow mapping (and even unmapping)
keys at any time and keep them alive by moving them into a trash can,
like we do for clients and others.
---
 src/keymap_manager.cc                            | 3 +++
 test/regression/4896-remap-executing-mapping/cmd | 3 +++
 test/regression/4896-remap-executing-mapping/in  | 1 +
 test/regression/4896-remap-executing-mapping/out | 1 +
 test/regression/4896-remap-executing-mapping/rc  | 3 +++
 5 files changed, 11 insertions(+)
 create mode 100644 test/regression/4896-remap-executing-mapping/cmd
 create mode 100644 test/regression/4896-remap-executing-mapping/in
 create mode 100644 test/regression/4896-remap-executing-mapping/out
 create mode 100644 test/regression/4896-remap-executing-mapping/rc

diff --git a/src/keymap_manager.cc b/src/keymap_manager.cc
index 81efe7ed..26579bf7 100644
--- a/src/keymap_manager.cc
+++ b/src/keymap_manager.cc
@@ -13,6 +13,9 @@ namespace Kakoune
 void KeymapManager::map_key(Key key, KeymapMode mode,
                             KeyList mapping, String docstring)
 {
+    if (auto it = m_mapping.find(KeyAndMode{key, mode}); it != m_mapping.end())
+        if (it->value.is_executing)
+            throw runtime_error("cannot map key that is currently executing");
     m_mapping[KeyAndMode{key, mode}] = {std::move(mapping), std::move(docstring)};
 }
 
diff --git a/test/regression/4896-remap-executing-mapping/cmd b/test/regression/4896-remap-executing-mapping/cmd
new file mode 100644
index 00000000..a09e364c
--- /dev/null
+++ b/test/regression/4896-remap-executing-mapping/cmd
@@ -0,0 +1,3 @@
+!printf $kak_opt_source_count<ret>l
+<space>s
+!printf $kak_opt_source_count<ret>
diff --git a/test/regression/4896-remap-executing-mapping/in b/test/regression/4896-remap-executing-mapping/in
new file mode 100644
index 00000000..8b137891
--- /dev/null
+++ b/test/regression/4896-remap-executing-mapping/in
@@ -0,0 +1 @@
+
diff --git a/test/regression/4896-remap-executing-mapping/out b/test/regression/4896-remap-executing-mapping/out
new file mode 100644
index 00000000..48082f72
--- /dev/null
+++ b/test/regression/4896-remap-executing-mapping/out
@@ -0,0 +1 @@
+12
diff --git a/test/regression/4896-remap-executing-mapping/rc b/test/regression/4896-remap-executing-mapping/rc
new file mode 100644
index 00000000..56624d18
--- /dev/null
+++ b/test/regression/4896-remap-executing-mapping/rc
@@ -0,0 +1,3 @@
+try %{ map global user s %exp{:source %%{%val{source}}<ret>} -docstring "re-source my kakrc" }
+declare-option int source_count
+set-option -add global source_count 1