fix: segfault in template caching logic#6421
Conversation
when templates had no executable requests after
option updates.
the cached templates could end up with 0 requests
and no flow execution path, resulting in a nil
engine pointer that was later derefer w/o
validation.
bug seq:
caching template (w/ valid requests) -> get cached
template -> `*ExecutorOptions.Options` copied and
modified (inconsistent) -> requests updated (with
new options -- some may be invalid, and without
recompile) -> template returned w/o validation ->
`compileProtocolRequests` -> `NewTemplateExecuter`
receive empty requests + empty flow = nil engine
-> `*TemplateExecuter.{Compile,Execute}` invoked
on nil engine = panic.
RCA:
1. `*ExecutorOptions.ApplyNewEngineOptions`
overwriting many fields.
2. copy op pointless; create a copy of options and
then immediately replace it with original
pointer.
3. missing executable requests validation after
cached templates is reconstructed with updated
options.
Thus, this affected `--automatic-scan` mode where
tech detection templates often have conditional
requests that may be filtered based on runtime
options.
Fixes #6417
Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughThe PR changes how engine options are applied to executors to avoid overwriting template-scoped state and updates template cache reuse to propagate Variables/Constants and validate cached templates before returning; cached templates lacking requests/workflows are re-parsed instead of reused. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Executor
participant EOpts as e.Options
participant NOpts as n.Options
Caller->>Executor: ApplyNewEngineOptions(n)
alt e.Options exists
Executor->>EOpts: Merge execution fields (ExecutionID, RateLimit, Timeout, Retries, etc.)
note right of EOpts: Template-scoped fields NOT overwritten
else e.Options is nil
Executor->>NOpts: e.Options = n.Options.Copy()
end
Executor->>Executor: Assign non-options refs (Output, IssuesClient, Progress, RateLimiter, Catalog, Browser, Interactsh, etc.)
sequenceDiagram
autonumber
participant Compiler
participant Cache
participant tpl as tplCopy
participant Caller
Compiler->>Cache: Lookup(template)
alt cache hit
Cache-->>Compiler: tplCopy
opt tplCopy.Options has Variables/Constants
Compiler->>Compiler: Propagate Variables/Constants into new base options
end
Compiler->>Compiler: Recompile using tplCopy.Options
Compiler->>Compiler: isCachedTemplateValid(tplCopy)?
alt valid
Compiler-->>Caller: Return cached/compiled template
else invalid
Compiler->>Compiler: Re-parse and compile from source
Compiler-->>Caller: Return freshly compiled template
end
else cache miss
Compiler->>Compiler: Parse and compile from source
Compiler-->>Caller: Return compiled template
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/templates/compile.go (1)
164-169: Workflow recompile uses the wrong options; loses propagated Variables/ConstantsYou propagate template metadata into
tplCopy.Options(vianewBase), but the workflow recompilation passesoptionsand then setsCompiledWorkflow.Options = options. That discardsVariables/Constantsand other template-scoped values you just preserved, and can lead to inconsistent runtime behavior in flow execution.Apply this diff to ensure the workflow uses the same options as the template:
- compileWorkflow(filePath, preprocessor, options, compiled, options.WorkflowLoader) + compileWorkflow(filePath, preprocessor, template.Options, compiled, template.Options.WorkflowLoader) - template.CompiledWorkflow.Options = options + template.CompiledWorkflow.Options = template.Options
🧹 Nitpick comments (2)
pkg/protocols/protocols.go (2)
445-449: Nit: typo in comment (“ExecuterOptions”)The type is
ExecutorOptions. Fix the comment to avoid confusion in future searches and docs.Apply this diff:
- // TODO: cached code|headless templates have nil ExecuterOptions if -code or -headless are not enabled + // TODO: cached code|headless templates have nil ExecutorOptions if -code or -headless are not enabled
450-460: Sync runtime gating toggles into e.OptionsMerging only execution-specific fields is the right direction. To avoid divergence between cached paths and fresh parses, also sync the runtime gating toggles that live on
types.Options. I confirmed thatHeadless,EnableCodeTemplates, andOfflineHTTPare defined ontypes.Optionsinpkg/types/types.go. As an optional refactor, extend the selective merge as follows:if e.Options != nil { e.Options.SetExecutionID(n.Options.GetExecutionID()) e.Options.RateLimit = n.Options.RateLimit e.Options.RateLimitDuration = n.Options.RateLimitDuration e.Options.Timeout = n.Options.Timeout e.Options.Retries = n.Options.Retries + // sync runtime gating toggles that influence executability + e.Options.Headless = n.Options.Headless + e.Options.EnableCodeTemplates = n.Options.EnableCodeTemplates + e.Options.OfflineHTTP = n.Options.OfflineHTTP } else { // The types.Options include the ExecutionID among other things e.Options = n.Options.Copy() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/protocols/protocols.go(1 hunks)pkg/templates/compile.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/templates/compile.go
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: In pkg/templates/compile.go, the template caching mechanism intentionally skips calling Compile() on copied requests to achieve performance benefits. This is the intended design, not a bug. The current implementation isn't production-ready but represents the desired direction.
Applied to files:
pkg/templates/compile.go
📚 Learning: 2025-07-16T21:28:08.073Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:59-78
Timestamp: 2025-07-16T21:28:08.073Z
Learning: The shallow copy behavior (`tplCopy := *value`) in pkg/templates/compile.go is intentional design for the template caching mechanism. The partial-copy approach is part of the performance optimization strategy, not a bug requiring deep copying.
Applied to files:
pkg/templates/compile.go
🧬 Code graph analysis (1)
pkg/protocols/protocols.go (1)
pkg/types/types.go (1)
Options(33-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (1)
pkg/templates/compile.go (1)
68-73: EnsureVariablesare deep-copied to prevent shared mutable stateOur investigation shows that
Variableembedsutils.InsertionOrderedStringMap, a mutable, insertion-ordered map type, and there is no existingCloneor copy helper onVariable(thergsearch for aClonemethod returned only the struct definition, no clone functions). Sharing this map across template/engine instances can lead to unintended cross-template mutations.• In
pkg/templates/compile.goat lines 68–73, after copyingConstants, also cloneVariablesrather than aliasing:- if tplCopy.Options.Variables.Len() > 0 { - newBase.Variables = tplCopy.Options.Variables - } + if tplCopy.Options.Variables.Len() > 0 { + // deep-copy to avoid aliasing mutable Variables across engines + newBase.Variables = tplCopy.Options.Variables.Clone() // implement Clone() + }• Since no
Cloneexists, add a helper onVariable(or directly onInsertionOrderedStringMap) to perform a deep copy of its internal map and key order before using it here.⛔ Skipped due to learnings
Learnt from: hdm PR: projectdiscovery/nuclei#6322 File: pkg/templates/compile.go:59-78 Timestamp: 2025-07-16T21:28:08.073Z Learning: The shallow copy behavior (`tplCopy := *value`) in pkg/templates/compile.go is intentional design for the template caching mechanism. The partial-copy approach is part of the performance optimization strategy, not a bug requiring deep copying.Learnt from: hdm PR: projectdiscovery/nuclei#6322 File: pkg/templates/compile.go:79-81 Timestamp: 2025-07-16T21:27:14.937Z Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/templates/compile.go (1)
171-175: Consider adding debug logging for cache invalidation casesWhile the validation logic is solid, it would be helpful to add debug logging when a cached template is invalidated to aid in troubleshooting cache misses.
if isCachedTemplateValid(template) { // options.Logger.Error().Msgf("returning cached template %s after recompiling %d requests", tplCopy.Options.TemplateID, tplCopy.Requests()) return template, nil +} else { + // Log why the cached template was invalidated for debugging + // options.Logger.Debug().Msgf("cached template %s invalidated, re-parsing from scratch", tplCopy.Options.TemplateID) } // else: fallthrough to re-parse template from scratch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/templates/compile.go(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/templates/compile.go
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: In pkg/templates/compile.go, the template caching mechanism intentionally skips calling Compile() on copied requests to achieve performance benefits. This is the intended design, not a bug. The current implementation isn't production-ready but represents the desired direction.
Applied to files:
pkg/templates/compile.go
📚 Learning: 2025-07-16T21:28:08.073Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:59-78
Timestamp: 2025-07-16T21:28:08.073Z
Learning: The shallow copy behavior (`tplCopy := *value`) in pkg/templates/compile.go is intentional design for the template caching mechanism. The partial-copy approach is part of the performance optimization strategy, not a bug requiring deep copying.
Applied to files:
pkg/templates/compile.go
🧬 Code graph analysis (1)
pkg/templates/compile.go (2)
pkg/templates/templates.go (1)
Template(36-165)pkg/protocols/protocols.go (1)
Executer(49-58)
🔇 Additional comments (5)
pkg/templates/compile.go (5)
68-73: Variables and Constants propagation looks goodThe addition of Variables and Constants propagation from the cached template's options to the new base options ensures that these metadata fields are preserved when reusing cached templates. This is important for maintaining template state during option updates.
166-168: Workflow compilation now uses consistent optionsGood fix - using
tplCopy.Optionsconsistently throughout the workflow compilation ensures that the same updated options are propagated to all levels of the workflow hierarchy.
171-174: Cache validation prevents nil executor issuesThe new validation check
isCachedTemplateValid(template)correctly prevents returning cached templates that would have nil executors or are otherwise invalid after option updates. This directly addresses the segfault issue where templates with zero executable requests were incorrectly returned from cache.
593-635: Comprehensive validation logic prevents invalid template usageThe
isCachedTemplateValidfunction provides thorough validation covering all the critical scenarios that could lead to nil executor issues:
- Checks for presence of requests/workflows
- Validates options initialization
- Ensures executor exists for non-workflow templates
- Ensures compiled workflow exists for workflow templates
- Verifies template ID consistency
- Includes sanity checks for executor state
The implementation correctly identifies and rejects cached templates that would cause runtime panics.
622-628: Validation logic for flow-enabled templates is sufficientThe
isCachedTemplateValidfunction already bypasses the zero-requests guard whenOptions.Flowis non-empty, and any flow compilation errors are surfaced immediately inNewTemplateExecuterbefore a template is ever cached. No additional invalid-state cases remain unhandled, so this check is adequate.
Signed-off-by: Dwi Siswanto <git@dw1.io>
3 participants
Proposed changes
when templates had no executable requests after
option updates.
the cached templates could end up with 0 requests
and no flow execution path, resulting in a nil
engine pointer that was later derefer w/o
validation.
bug seq:
caching template (w/ valid requests) -> get cached
template ->
*ExecutorOptions.Optionscopied andmodified (inconsistent) -> requests updated (with
new options -- some may be invalid, and without
recompile) -> template returned w/o validation ->
compileProtocolRequests->NewTemplateExecuterreceive empty requests + empty flow = nil engine
->
*TemplateExecuter.{Compile,Execute}invokedon nil engine = panic.
RCA:
*ExecutorOptions.ApplyNewEngineOptionsoverwriting many fields.
then immediately replace it with original
pointer.
cached templates is reconstructed with updated
options.
Thus, this affected
--automatic-scanmode wheretech detection templates often have conditional
requests that may be filtered based on runtime
options.
Fixes #6417
Checklist
Summary by CodeRabbit