fix(runtime): crashes and hangs from choice/thread scoping bugs #170

.github/workflows/build.yml

@@ -47,6 +47,13 @@ jobs:
         with:
           submodules: true
 
+      # Windows Defender`s real-time scanning can lock/delay the visibility of freshly written files.
+      # This can lead to faling tests (since proofing creates many files and read them imediat)
+      - name: Exclude workspace from Windows Defender scanning
+        if: ${{ matrix.os == 'windows-latest' }}
+        shell: pwsh
+        run: Add-MpPreference -ExclusionPath "${{ github.workspace }}"
+
       # Setup python
       - uses: actions/setup-python@v7
         if: ${{ matrix.proof }}

.github/workflows/pr-report.yml

@@ -29,7 +29,7 @@ jobs:
       # Retrieve the PR number from the workflow run
       - name: Get PR number
         id: get-pr
-        uses: actions/github-script@v7
+        uses: actions/github-script@v9
         with:
           script: |
             const prs = await github.rest.pulls.list({

inkcpp/array.h

@@ -247,6 +247,23 @@ class managed_restorable_array : public managed_array<T, dynamic, initialCapacit
 
 	void forgett() { _last_size = 0; }
 
+	/** Moves the save pointer backwards, to allow reclasificaion.
+	 * Only should move past uncommited elements.
+	 * @param position to rebase on
+	 * @param is_pending predicate to validate that elements are non commited
+	 */
+	template<typename Pred>
+	void rebase_save(size_t position, Pred is_pending)
+	{
+		if (! is_saved() || position >= _last_size) {
+			return;
+		}
+		for (size_t i = position; i < _last_size; ++i) {
+			inkAssert(is_pending(i), "Cannot rebase save point over already committed data.");
+		}
+		_last_size = position;
+	}
+
 	bool has_changed() const { return base::size() != _last_size; }
 
 	size_t last_size() const { return _last_size; }
@@ -670,4 +687,94 @@ inline const unsigned char* allocated_restorable_array<
 	return ptr;
 }
 
+/** A managed @ref managed_array sibling class if pointers to element should be handed.
+ * If in dynamic mode, does allocate a new chunk of data each time the limit is reached, but keep
+ * the old one for the old data.
+ * If in static mode behaves like a static managed_array
+ * @attention cannot be snapshoted!
+ */
+template<typename T, int config>
+class managed_instances
+{
+	static constexpr bool   dynamic = config < 0;
+	static constexpr size_t fixed_n = dynamic ? 0 : static_cast<size_t>(config);
+
+public:
+	managed_instances()
+	{
+		if constexpr (dynamic) {
+			T* block = reinterpret_cast<T*>(new (std::nothrow) unsigned char[sizeof(T) * abs(config)]);
+			inkAssert(block != nullptr, "Out of memory in inkcpp: managed_instances grow failed");
+			inkAssert(( ::size_t ) block % alignof(T) == 0, "Miss aligend data in managed_instances");
+			for (size_t i = 0; i < abs(config); ++i) {
+				_blocks.push() = block + i;
+			}
+		}
+	}
+
+	managed_instances(const managed_instances&)            = delete;
+	managed_instances& operator=(const managed_instances&) = delete;
+
+	~managed_instances()
+	{
+		if constexpr (dynamic) {
+			for (size_t i = 0; i < _blocks.size(); i = i == 0 ? abs(config) : i + next_chunk_size(i)) {
+				delete[] reinterpret_cast<unsigned char*>(_blocks[i]);
+			}
+		}
+	}
+
+	T& push()
+	{
+		if constexpr (dynamic) {
+			if (_active == _blocks.size()) {
+				const size_t n_new_elements = next_chunk_size(_blocks.size());
+				T*           block
+				    = reinterpret_cast<T*>(new (std::nothrow) unsigned char[sizeof(T) * n_new_elements]);
+				inkAssert(block != nullptr, "Out of memory in inkcpp: managed_instances grow failed");
+				inkAssert(( ::size_t ) block % alignof(T) == 0, "Miss aligend data in managed_instances");
+				for (size_t i = 0; i < n_new_elements; ++i) {
+					_blocks.push() = block + i;
+				}
+			}
+			return *_blocks[_active++];
+		} else {
+			inkAssert(_active < fixed_n, "managed_instaces is full, and is static");
+			T* slot = reinterpret_cast<T*>(_storage + _active * sizeof(T));
+			++_active;
+			return *slot;
+		}
+	}
+
+	/** Marks every handou-out slot as free again.
+	 * @attention The memory is retained
+	 */
+	void clear() { _active = 0; }
+
+	config::statistics::container statistics() const
+	{
+		if constexpr (dynamic) {
+			return {static_cast<int>(_blocks.size()), static_cast<int>(_active)};
+		} else {
+			return {static_cast<int>(fixed_n), static_cast<int>(_active)};
+		}
+	}
+
+private:
+	/** Number of elements allocate for the next chunk.
+	 * @param total_allocated total number of currently allocated slots
+	 */
+	static constexpr size_t next_chunk_size(size_t total_allocated)
+	{
+		size_t n = total_allocated / 2;
+		return n == 0 ? 1 : n;
+	}
+
+	size_t _active = 0;
+	alignas(
+	    dynamic ? alignof(unsigned char) : alignof(T)
+	) unsigned char _storage[dynamic ? 1 : fixed_n * sizeof(T)];
+	managed_array<T*, dynamic, dynamic ? abs(config) : 1, true> _blocks;
+};
+
 } // namespace ink::runtime::internal

inkcpp/choice.cpp

@@ -7,6 +7,7 @@
 #include "choice.h"
 
 #include "output.h"
+#include "runner_impl.h"
 #include "snapshot_impl.h"
 #include "string_table.h"
 #include "string_utils.h"
@@ -16,30 +17,30 @@ namespace ink
 namespace runtime
 {
 
-	size_t choice::num_tags() const
-	{
-		return static_cast<size_t>(std::distance(_tags_start, _tags_end));
-	}
+	size_t choice::num_tags() const { return _tags_end - _tags_start; }
 
 	const char* choice::get_tag(size_t index) const
 	{
-		return (index < num_tags()) ? _tags_start[index].text() : nullptr;
+		return (index < num_tags()) ? _runner->tag_text(_tags_start + index) : nullptr;
 	}
 
 	choice& choice::setup(
 	    internal::basic_stream& in, internal::string_table& strings, internal::list_table& lists,
-	    int index, uint32_t path, thread_t thread, const internal::snap_tag* tags_start,
-	    const internal::snap_tag* tags_end
+	    int index, uint32_t path, thread_t thread, thread_t scope_thread,
+	    const internal::runner_impl& runner, size_t tags_start, size_t tags_end
 	)
 	{
 		// Index/path
-		_index      = index;
-		_path       = path;
-		_thread     = thread;
-		_tags_start = tags_start;
-		_tags_end   = tags_end;
+		_index        = index;
+		_path         = path;
+		_thread       = thread;
+		_scope_thread = scope_thread;
+		_runner       = &runner;
+		_tags_start   = tags_start;
+		_tags_end     = tags_end;
 
 		char* text = nullptr;
+		in.commit_marker_extraction();
 		// if we only have one item in our output stream
 		if (in.queued() == 2) {
 			// If it's a string, just grab it. Otherwise, use allocation

inkcpp/include/choice.h

@@ -17,7 +17,6 @@ namespace runtime
 		class runner_impl;
 		class string_table;
 		class list_table;
-		class snap_tag;
 	} // namespace internal
 
 	/**
@@ -69,17 +68,19 @@ namespace runtime
 
 		choice& setup(
 		    internal::basic_stream&, internal::string_table& strings, internal::list_table& lists,
-		    int index, uint32_t path, thread_t thread, const internal::snap_tag* tags_start,
-		    const internal::snap_tag* tags_end
+		    int index, uint32_t path, thread_t thread, thread_t scope_thread,
+		    const internal::runner_impl& runner, size_t tags_start, size_t tags_end
 		);
 
 	protected:
-		const char*               _text       = nullptr; ///< @private
-		const internal::snap_tag* _tags_start = nullptr; ///< @private
-		const internal::snap_tag* _tags_end   = nullptr; ///< @private
-		uint32_t                  _path       = ~0U;     ///< @private
-		thread_t                  _thread     = ~0U;     ///< @private
-		int                       _index      = -1;      ///< @private
+		const char*                  _text       = nullptr; ///< @private
+		const internal::runner_impl* _runner     = nullptr; ///< @private
+		size_t                       _tags_start = 0;       ///< @private
+		size_t                       _tags_end   = 0;       ///< @private
+		uint32_t                     _path       = ~0U;     ///< @private
+		thread_t                     _thread     = ~0U;     ///< @private
+		thread_t _scope_thread = ~0U; ///< Used to avoid variable pollution from choices. @private
+		int      _index        = -1;  ///< @private
 	};
 
 } // namespace runtime

inkcpp/include/runner.h

@@ -379,6 +379,8 @@ class runner_interface
 	 *
 	 * Returns the choice object at a given index
 	 *
+	 * @attention the returned pointer is only valid until @ref choose is called next; do not
+	 * retain it across a choice being made.
 	 * @see num_choices
 	 * @param index index of the choice to access
 	 * @return choice object with info on the choice
@@ -388,6 +390,8 @@ class runner_interface
 	/**
 	 * Shorcut for accessing a choice
 	 *
+	 * @attention the returned pointer is only valid until @ref choose is called next; do not
+	 * retain it across a choice being made.
 	 * @see get_choice
 	 * @param index index of the choice to access
 	 * @return choice object with info on the choice

inkcpp/include/types.h

@@ -147,7 +147,7 @@ struct value {
 
 	/// @}
 
-#ifdef __GNUCC__
+#ifdef __GNUC__
 #	pragma GCC diagnostic push
 #	pragma GCC diagnostic ignored "-Wtautological-compare"
 #endif
@@ -160,7 +160,7 @@ struct value {
 	{
 		static_assert(Ty != Ty, "No value getter for the selected type");
 	}
-#ifdef __GNUCC__
+#ifdef __GNUC__
 #	pragma GCC diagnostic pop
 #endif
 };

inkcpp/list_table.cpp

@@ -865,9 +865,9 @@ list_flag list_table::get_list_id(const char* list_name) const
 
 list_table::list list_table::redefine(list lh, list rh)
 {
+	list    res = create();
 	data_t* l   = getPtr(lh.lid);
 	data_t* r   = getPtr(rh.lid);
-	list    res = create();
 	data_t* o   = getPtr(res.lid);
 
 	// if the new list has no origin: give it the origin of the old value

inkcpp/list_table.h

@@ -11,6 +11,8 @@
 #include "array.h"
 #include "./include/list.h"
 
+#include <new>
+
 #ifdef INK_ENABLE_STL
 #	include <iosfwd>
 #endif
@@ -469,12 +471,12 @@ class list_table : public snapshot_interface
 	);
 
 	// defined list (meta data)
-	managed_array<size_t, config::maxListTypes>                     _list_end;
-	managed_array<const char*, config::maxFlags>                    _flag_names;
-	managed_array<int, config::maxFlags>                            _flag_values;
-	managed_array<const char*, config::maxListTypes>                _list_names;
+	managed_array<size_t, config::maxListTypes>                   _list_end;
+	managed_array<const char*, config::maxFlags>                  _flag_names;
+	managed_array<int, config::maxFlags>                          _flag_values;
+	managed_array<const char*, config::maxListTypes>              _list_names;
 	/// keep track over lists accessed with get_var, and clear then at gc time
-	managed_array<list_interface, config::limitEditableLists, true> _list_handouts;
+	managed_instances<list_interface, config::limitEditableLists> _list_handouts;
 
 	bool _valid;
 

inkcpp/output.cpp

@@ -229,7 +229,7 @@ FString basic_stream::get()
 }
 #endif
 
-size_t basic_stream::queued() const
+size_t basic_stream::queued()
 {
 	size_t start = find_start();
 	return _size - start;
@@ -277,16 +277,7 @@ void basic_stream::get(value* ptr, size_t length)
 
 size_t basic_stream::find_first_of(value_type type, size_t offset /*= 0*/) const
 {
-	if (_size == 0)
-		return npos;
-
-	// TODO: Cache?
-	for (size_t i = offset; i < _size; ++i) {
-		if (_data[i].type() == type)
-			return i;
-	}
-
-	return npos;
+	return find_first_of([type](const value& v) { return v.type() == type; }, offset);
 }
 
 size_t basic_stream::find_last_of(value_type type, size_t offset /*= 0*/) const
@@ -338,6 +329,13 @@ void basic_stream::forget()
 	_save = npos;
 }
 
+void basic_stream::rebase_save(size_t position)
+{
+	inkAssert(saved(), "No save point to rebase!");
+	inkAssert(position <= _save, "Can not move save point forward!");
+	_save = position;
+}
+
 template char* basic_stream::get_alloc<true>(string_table& strings, list_table& lists);
 template char* basic_stream::get_alloc<false>(string_table& strings, list_table& lists);
 
@@ -402,7 +400,7 @@ char* basic_stream::get_alloc(string_table& strings, list_table& lists)
 				break;
 			case value_type::list: ptr = lists.toString(ptr, _data[i].get<value_type::list>()); break;
 			case value_type::list_flag:
-				ptr = lists.toString(ptr, _data[i].get<value_type::list>());
+				ptr = lists.toString(ptr, _data[i].get<value_type::list_flag>());
 				break;
 			default: inkFail("cant convert expression to string!");
 		}
@@ -430,13 +428,20 @@ size_t basic_stream::find_start() const
 {
 	// Find marker (or start)
 	size_t start = _size;
+	bool   found = false;
 	while (start > 0) {
 		start--;
-		if (_data[start].type() == value_type::marker)
+		if (_data[start].type() == value_type::marker) {
+			found = true;
 			break;
+		}
 	}
 
-	// Make sure we're not violating a save point
+	// If the marker was already consumed, the save point is the start of the
+	// remaining output and data before it must not be extracted.
+	if (saved() && ! found) {
+		start = _save;
+	}
 	if (saved() && start < _save) {
 		// TODO: check if we don't reset save correct
 		// at some point we can modifiy the output even behind save (probally discard?) and push a new
@@ -447,6 +452,14 @@ size_t basic_stream::find_start() const
 	return start;
 }
 
+void basic_stream::commit_marker_extraction()
+{
+	const size_t marker = find_first_of(value_type::marker);
+	if (marker != npos && saved() && marker < _save) {
+		rebase_save(marker);
+	}
+}
+
 bool basic_stream::should_skip(size_t iter, bool& hasGlue, bool& lastNewline) const
 {
 	if (_data[iter].printable() && _data[iter].type() != value_type::newline