fix : Dangling pointer for non-empty trigger words in lazy grammar construction

[marek-hradil]

Fix on issue #17047.

Added static keyword, to not let the pointer be out of scope. However, my understanding of this functionality in cpp is poor, so happy to be corrected with some better engineering.

To consider: Isn't static also needed on line 1631, when constructing trigger_pattern?

[ggerganov]

Yes, looks like a bug. But adding static is not the correct solution.

I think that simply keeping trigger_patterns in scope until grammar init is completed, should be enough.

[marek-hradil]

@ggerganov that makes much more sense. I've edited that, can you have a look if it looks better now?

[Green-Sky]

This is still a dangling pointer.

[marek-hradil]

@Green-Sky Hi, can you provide more info why this is still dangling? (im happy to fix, but im really not knowledgeable in cpp, so id appreciate it greatly)

My reasoning:

1. trigger_pattern and trigger_pattern_c are declared outside of the if scope
2. in the if, the trigger_patterns is set to a pointer to the trigger_pattern
3. then in llama_grammar_init_impl, everything should still exist fine
4. on line 1061 in llama-grammar, the patterns are looped through and then accessed and copied with:

trigger.pattern = trigger_patterns[i]

This way, we make copy for the ctx object and can forget the rest.

[Green-Sky]

trigger_patterns lives outside the function, so it will outlive all variables you might declare inside the function. So when we assign it a pointer from something with a lower scope (within the function), the pointer will outlive it.
When I looked last, the call stack for that function is somewhat convoluted...

(im happy to fix, but im really not knowledgeable in cpp, so id appreciate it greatly)

No worries, you can ask anything about cpp. :)

[o7si]

Hi, the usage of const char ** trigger_patterns here is somewhat subtle, but I think trigger_patterns appears to be safe :D, because it only modifies a local variable without affecting the passed array (as demonstrated in init_v1 below). However, if implemented like init_v2, it would result in a dangling pointer.

#include <iostream>
#include <string>
#include <vector>


void init_v1(const char ** trigger_patterns) {

    std::string str = "hello";
    const char * c_str = str.c_str();

    // SAFE: Only modifies local parameter, doesn't affect caller's array
    trigger_patterns = &c_str;
}

void init_v2(const char ** trigger_patterns) {

    std::string str = "hello";
    const char * c_str = str.c_str();

    // UNSAFE: Modifies caller's array with dangling pointer (str is destroyed after return)
    *trigger_patterns = c_str;
}

int main(int argc, char ** argv) {

    std::vector<std::string> trigger_patterns = {
        "mock1", "mock2", "mock3",
    };

    std::vector<const char *> trigger_patterns_c;
    trigger_patterns_c.reserve(trigger_patterns.size());
    for (const auto & x : trigger_patterns) {
        trigger_patterns_c.push_back(x.c_str());
    }

    init_v1(trigger_patterns_c.data());
    for (const auto & x : trigger_patterns_c) {
        // 0x13ce06000 mock1
        // 0x13ce06008 mock2
        // 0x13ce06010 mock3
        std::cout << &x << " " << x << std::endl;
    }

    init_v2(trigger_patterns_c.data());
    for (const auto & x : trigger_patterns_c) {
        // 0x13ce06000 �d�m
        // 0x13ce06008 mock2
        // 0x13ce06010 mock3
        std::cout << &x << " " << x << std::endl;
    }

    return 0;
}

Perhaps we could use the following implementation approach, which offers better readability and avoids the subtle behavior:

diff --git a/src/llama-sampling.cpp b/src/llama-sampling.cpp
index 55d2e355f..2f32401f6 100644
--- a/src/llama-sampling.cpp
+++ b/src/llama-sampling.cpp
@@ -1626,6 +1626,7 @@ static struct llama_sampler * llama_sampler_init_grammar_impl(
 
     if (grammar_str != nullptr && grammar_str[0] != '\0') {
         // TODO: remove trigger_words support.
+        llama_grammar * grammar = nullptr;
         if (trigger_words != nullptr && num_trigger_words > 0) {
             GGML_ASSERT(trigger_patterns == nullptr && num_trigger_patterns == 0);
             std::string trigger_pattern("[\\s\\S]*?(");
@@ -1637,15 +1638,17 @@ static struct llama_sampler * llama_sampler_init_grammar_impl(
                 trigger_pattern += std::regex_replace(trigger_words[i], special_chars, "\\$0");
             }
             trigger_pattern += ")[\\s\\S]*";
-            const auto * trigger_pattern_c = trigger_pattern.c_str();
-            trigger_patterns = &trigger_pattern_c;
-            num_trigger_patterns = 1;
+            std::vector<const char *> trigger_patterns_c;
+            trigger_patterns_c.push_back(trigger_pattern.c_str());
+            grammar = llama_grammar_init_impl(vocab, grammar_str, grammar_root, lazy, trigger_patterns_c.data(), trigger_patterns_c.size(), trigger_tokens, num_trigger_tokens);
+        } else {
+            grammar = llama_grammar_init_impl(vocab, grammar_str, grammar_root, lazy, trigger_patterns, num_trigger_patterns, trigger_tokens, num_trigger_tokens);
         }
         *ctx = {
             /* .vocab        = */ vocab,
             /* .grammar_str  = */ grammar_str,
             /* .grammar_root = */ grammar_root,
-            /* .grammar      = */ llama_grammar_init_impl(vocab, grammar_str, grammar_root, lazy, trigger_patterns, num_trigger_patterns, trigger_tokens, num_trigger_tokens),
+            /* .grammar      = */ grammar,
         };
         if (!ctx->grammar) {
             delete ctx;

If there are any errors in my code, please correct me. Thank you for the guidance!

#17048 (comment)