From 08c29d52c59cf0be952113f5ef53abdb917f7d32 Mon Sep 17 00:00:00 2001 From: Martin Evans Date: Sat, 4 Nov 2023 16:02:33 +0000 Subject: [PATCH 1/2] Slightly refactored `SafeLLamaGrammarHandle.Create` to solve CodeQL warning about pointer arithmetic. --- LLama/Native/SafeLLamaGrammarHandle.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/LLama/Native/SafeLLamaGrammarHandle.cs b/LLama/Native/SafeLLamaGrammarHandle.cs index f430b7c3f..2218995b2 100644 --- a/LLama/Native/SafeLLamaGrammarHandle.cs +++ b/LLama/Native/SafeLLamaGrammarHandle.cs @@ -49,29 +49,31 @@ public static SafeLLamaGrammarHandle Create(IReadOnlyList rules, ul // Borrow an array large enough to hold every single element // and another array large enough to hold a pointer to each rule var allElements = ArrayPool.Shared.Rent(totalElements); - var pointers = ArrayPool.Shared.Rent(rules.Count); + var rulePointers = ArrayPool.Shared.Rent(rules.Count); try { fixed (LLamaGrammarElement* allElementsPtr = allElements) { var elementIndex = 0; - var pointerIndex = 0; + var ruleIndex = 0; foreach (var rule in rules) { + Debug.Assert(elementIndex < allElements.Length); + // Save a pointer to the start of this rule - pointers[pointerIndex++] = (IntPtr)(allElementsPtr + elementIndex); + rulePointers[ruleIndex++] = (IntPtr)(allElementsPtr + elementIndex); // Copy all of the rule elements into the flat array foreach (var element in rule.Elements) - allElementsPtr[elementIndex++] = element; + allElements[elementIndex++] = element; } // Sanity check some things that should be true if the copy worked as planned - Debug.Assert((ulong)pointerIndex == nrules); + Debug.Assert((ulong)ruleIndex == nrules); Debug.Assert(elementIndex == totalElements); // Make the actual call through to llama.cpp - fixed (void* ptr = pointers) + fixed (void* ptr = rulePointers) { return Create((LLamaGrammarElement**)ptr, nrules, start_rule_index); } @@ -80,7 +82,7 @@ public static SafeLLamaGrammarHandle Create(IReadOnlyList rules, ul finally { ArrayPool.Shared.Return(allElements); - ArrayPool.Shared.Return(pointers); + ArrayPool.Shared.Return(rulePointers); } } } From a03fdc4818acaca6d49d5ad027d26bb42c006dc3 Mon Sep 17 00:00:00 2001 From: Martin Evans Date: Sat, 4 Nov 2023 16:17:32 +0000 Subject: [PATCH 2/2] Using a reference to an array instead of pointer arithmetic. This means it will benefit from bounds checking on the array. --- LLama/Native/SafeLLamaGrammarHandle.cs | 42 +++++++++++++------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/LLama/Native/SafeLLamaGrammarHandle.cs b/LLama/Native/SafeLLamaGrammarHandle.cs index 2218995b2..ee27befd2 100644 --- a/LLama/Native/SafeLLamaGrammarHandle.cs +++ b/LLama/Native/SafeLLamaGrammarHandle.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Runtime.CompilerServices; using LLama.Exceptions; using LLama.Grammars; @@ -52,31 +53,30 @@ public static SafeLLamaGrammarHandle Create(IReadOnlyList rules, ul var rulePointers = ArrayPool.Shared.Rent(rules.Count); try { - fixed (LLamaGrammarElement* allElementsPtr = allElements) - { - var elementIndex = 0; - var ruleIndex = 0; - foreach (var rule in rules) - { - Debug.Assert(elementIndex < allElements.Length); + // We're taking pointers into `allElements` below, so this pin is required to fix + // that memory in place while those pointers are in use! + using var pin = allElements.AsMemory().Pin(); - // Save a pointer to the start of this rule - rulePointers[ruleIndex++] = (IntPtr)(allElementsPtr + elementIndex); + var elementIndex = 0; + var ruleIndex = 0; + foreach (var rule in rules) + { + // Save a pointer to the start of this rule + rulePointers[ruleIndex++] = (IntPtr)Unsafe.AsPointer(ref allElements[elementIndex]); - // Copy all of the rule elements into the flat array - foreach (var element in rule.Elements) - allElements[elementIndex++] = element; - } + // Copy all of the rule elements into the flat array + foreach (var element in rule.Elements) + allElements[elementIndex++] = element; + } - // Sanity check some things that should be true if the copy worked as planned - Debug.Assert((ulong)ruleIndex == nrules); - Debug.Assert(elementIndex == totalElements); + // Sanity check some things that should be true if the copy worked as planned + Debug.Assert((ulong)ruleIndex == nrules); + Debug.Assert(elementIndex == totalElements); - // Make the actual call through to llama.cpp - fixed (void* ptr = rulePointers) - { - return Create((LLamaGrammarElement**)ptr, nrules, start_rule_index); - } + // Make the actual call through to llama.cpp + fixed (void* ptr = rulePointers) + { + return Create((LLamaGrammarElement**)ptr, nrules, start_rule_index); } } finally