Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −2
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cc7c8a
fix : Dangling pointer for non-empty trigger words in llama_sampler_i…
marek-hradil Nov 6, 2025
f846c83
Replace 'static' workaround, with keeping variable in scope for longer
marek-hradil Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/llama-sampling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1625,10 +1625,12 @@ static struct llama_sampler * llama_sampler_init_grammar_impl(
auto * ctx = new llama_sampler_grammar;

if (grammar_str != nullptr && grammar_str[0] != '\0') {
std::string trigger_pattern;
const char * trigger_pattern_c = nullptr;
// TODO: remove trigger_words support.
if (trigger_words != nullptr && num_trigger_words > 0) {
GGML_ASSERT(trigger_patterns == nullptr && num_trigger_patterns == 0);
std::string trigger_pattern("[\\s\\S]*?(");
trigger_pattern = "[\\s\\S]*?(";
for (size_t i = 0; i < num_trigger_words; ++i) {
static const std::regex special_chars("[.^$|()*+?\\[\\]{}\\\\]");
if (i > 0) {
Expand All @@ -1637,7 +1639,7 @@ 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_pattern_c = trigger_pattern.c_str();
trigger_patterns = &trigger_pattern_c;
Copy link
Collaborator

@Green-Sky Green-Sky Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a dangling pointer.

Copy link
Author

@marek-hradil marek-hradil Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

@Green-Sky Green-Sky Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link

@o7si o7si Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@Green-Sky Green-Sky Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o7si Thanks, you are fully correct. It is not actually a dangling pointer. 👍

Your change proposal reads good. I would not have used a std::vector<> as a temporary though. Either use std::array<T, 1> or just a c array (of const char pointers).

num_trigger_patterns = 1;
}
Expand Down
Loading