fix(loader): avoid panic when dialers are missing#7027
Open
Crucis918 wants to merge 4 commits intoprojectdiscovery:devfrom
Open
fix(loader): avoid panic when dialers are missing#7027Crucis918 wants to merge 4 commits intoprojectdiscovery:devfrom
Crucis918 wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
Contributor
WalkthroughLoadTemplatesWithTags no longer panics when no dialers exist for an execution ID. It attempts to initialize dialers via Changes
Sequence Diagram(s)sequenceDiagram
participant Loader
participant ProtocolState
participant Dialers
Loader->>ProtocolState: GetDialersWithId(executionId)
alt dialers found
ProtocolState-->>Loader: dialers
Loader->>Loader: create wait group and proceed with concurrent loading
else no dialers
ProtocolState-->>Loader: nil
Loader->>ProtocolState: Init(typesOpts)
alt init success and dialers now exist
ProtocolState-->>Loader: dialers
Loader->>Loader: create wait group and proceed
else init failed or still nil
ProtocolState-->>Loader: nil / error
Loader-->>Loader: log error and return empty slice
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/catalog/loader/loader.go (1)
709-712: Wait group allocated before the dialers guard — minor ordering nit.
wgLoadTemplatesis created at lines 709–712 before theexecutionId/dialers checks at lines 714–722. If dialers are nil the wait group is created and immediately discarded. Moving the guard before the wait group creation avoids the unnecessary allocation on the error path.♻️ Proposed reorder
+ if typesOpts.ExecutionId == "" { + typesOpts.ExecutionId = xid.New().String() + } + + if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { + store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId) + return nil + } + wgLoadTemplates, errWg := syncutil.New(syncutil.WithSize(concurrency)) if errWg != nil { panic("could not create wait group") } - - if typesOpts.ExecutionId == "" { - typesOpts.ExecutionId = xid.New().String() - } - - dialers := protocolstate.GetDialersWithId(typesOpts.ExecutionId) - if dialers == nil { - store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId) - return nil - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader.go` around lines 709 - 712, The wait group wgLoadTemplates is allocated via syncutil.New before the guard that checks executionId and dialers, causing an unnecessary allocation if dialers are nil; move the syncutil.New(...) call (and its error handling) so it occurs after the executionId/dialers validation (the guard around executionId and dialers) to avoid creating wgLoadTemplates when the function will early-return or panic due to missing dialers; update references to wgLoadTemplates and errWg accordingly so the variable is only created after the validation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 720-721: The current early return in LoadTemplatesWithTags
(store.logger.Error().Msgf(...); return nil) hides configuration errors by
returning nil instead of an error; change LoadTemplatesWithTags to return
([]Template, error) and return a descriptive error (e.g., fmt.Errorf with
executionId context) instead of nil, then update all callers (LoadTemplates,
Load, lazy auth loading, tech detection paths, etc.) to accept and propagate
that error up the call chain; if you intentionally want to avoid the refactor,
add a clear code comment at the top of LoadTemplatesWithTags documenting the
deliberate deviation and the risk of silent failures so future readers know why
error propagation was not implemented.
- Around line 718-722: The fetch of dialers via
protocolstate.GetDialersWithId(typesOpts.ExecutionId) creates a variable dialers
that is never used; either make this an explicit precondition check or actually
pass the dialers into downstream logic. Fix by replacing the unused variable
with an inline existence check (e.g., if
protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil {
store.logger.Error()...; return nil }) if you only need validation, OR wire
dialers into the downstream calls that need executor information (e.g., attach
dialers to store.config.ExecutorOptions or pass it into the template
parsing/loader functions that currently receive store.config.ExecutorOptions,
store.preprocessor, and store.tagFilter). Ensure you update references to
dialers (or remove the variable) in the function to eliminate the dead
assignment.
---
Nitpick comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 709-712: The wait group wgLoadTemplates is allocated via
syncutil.New before the guard that checks executionId and dialers, causing an
unnecessary allocation if dialers are nil; move the syncutil.New(...) call (and
its error handling) so it occurs after the executionId/dialers validation (the
guard around executionId and dialers) to avoid creating wgLoadTemplates when the
function will early-return or panic due to missing dialers; update references to
wgLoadTemplates and errWg accordingly so the variable is only created after the
validation succeeds.
pkg/catalog/loader/loader.go
Outdated
Comment on lines
720
to
721
| store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId) | ||
| return nil |
Contributor
There was a problem hiding this comment.
Returning nil is semantically ambiguous and diverges from the issue's requested error-propagation design.
Issue #6674 explicitly requested updating LoadTemplatesWithTags's signature to propagate an error, and updating callers accordingly. The current change returns nil — which is indistinguishable from "zero templates matched" at every call site (e.g., LoadTemplates → Load(), lazy auth loading, tech detection paths). Callers silently proceed with zero templates and have no way to detect a misconfigured execution ID vs. a legitimate empty result.
The logging provides a human signal but no programmatic signal. Consider honouring the original design request:
♻️ Proposed signature change (sketch)
-func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) []*templates.Template {
+func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) ([]*templates.Template, error) {
defer store.saveMetadataIndexOnce()
...
- if dialers == nil {
- store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId)
- return nil
- }
+ if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil {
+ return nil, fmt.Errorf("dialers with executionId %s not found", typesOpts.ExecutionId)
+ }
...
- return loadedTemplates.Slice
+ return loadedTemplates.Slice, nil
}LoadTemplates, Load, and other callers would then need to be updated to handle and propagate the error.
If the current "log and return nil" approach is intentional to avoid a larger refactor, the PR description and/or a code comment should document the deviation and the risk of silent failures at call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/catalog/loader/loader.go` around lines 720 - 721, The current early
return in LoadTemplatesWithTags (store.logger.Error().Msgf(...); return nil)
hides configuration errors by returning nil instead of an error; change
LoadTemplatesWithTags to return ([]Template, error) and return a descriptive
error (e.g., fmt.Errorf with executionId context) instead of nil, then update
all callers (LoadTemplates, Load, lazy auth loading, tech detection paths, etc.)
to accept and propagate that error up the call chain; if you intentionally want
to avoid the refactor, add a clear code comment at the top of
LoadTemplatesWithTags documenting the deliberate deviation and the risk of
silent failures so future readers know why error propagation was not
implemented.
Author
|
Applied CodeRabbit nit: moved wait group initialization after executionId/dialers guard to avoid unnecessary allocation on error path. Local check: go test ./pkg/catalog/loader |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/catalog/loader/loader.go (1)
713-717:⚠️ Potential issue | 🟠 MajorReturn-type semantics are still ambiguous on missing dialers.
At Line 716, returning
nilstill hides a configuration/runtime error as a normal “no templates” result. This path should return an error so callers can distinguish failure from empty matches and react appropriately.♻️ Suggested direction
-func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) []*templates.Template { +func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) ([]*templates.Template, error) { ... - if dialers == nil { - store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId) - return nil - } + if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { + return nil, fmt.Errorf("dialers with executionId %s not found", typesOpts.ExecutionId) + } ... - return loadedTemplates.Slice + return loadedTemplates.Slice, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader.go` around lines 713 - 717, The code currently logs "dialers with executionId %s not found" and returns nil from the block that checks protocolstate.GetDialersWithId(typesOpts.ExecutionId); change this to return a non-nil error so callers can distinguish a failure to find dialers from a legitimate "no templates" result. Concretely, replace the lone nil return with a descriptive error (e.g., fmt.Errorf or errors.Wrap) that includes typesOpts.ExecutionId, keep the log line, and ensure the enclosing function's return signature and all callers of the enclosing function are updated to handle the error return; reference symbols: protocolstate.GetDialersWithId, typesOpts.ExecutionId, and store.logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 713-717: The code currently logs "dialers with executionId %s not
found" and returns nil from the block that checks
protocolstate.GetDialersWithId(typesOpts.ExecutionId); change this to return a
non-nil error so callers can distinguish a failure to find dialers from a
legitimate "no templates" result. Concretely, replace the lone nil return with a
descriptive error (e.g., fmt.Errorf or errors.Wrap) that includes
typesOpts.ExecutionId, keep the log line, and ensure the enclosing function's
return signature and all callers of the enclosing function are updated to handle
the error return; reference symbols: protocolstate.GetDialersWithId,
typesOpts.ExecutionId, and store.logger.
Author
|
Addressed CodeRabbit note about semantics:
Local check: go test ./pkg/catalog/loader |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/catalog/loader/loader.go (1)
715-723:⚠️ Potential issue | 🟠 MajorDialer-init failures are still silently collapsed into “no templates”.
At Line 717 and Line 722, returning an empty slice makes “dialer setup failed” indistinguishable from “no templates matched”. This still prevents callers from reacting programmatically to execution-id misconfiguration.
♻️ Suggested direction
-func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) []*templates.Template { +func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) ([]*templates.Template, error) { ... - return []*templates.Template{} + return nil, fmt.Errorf("dialers init failed for executionId %s: %w", typesOpts.ExecutionId, err) ... - return []*templates.Template{} + return nil, fmt.Errorf("dialers with executionId %s not found", typesOpts.ExecutionId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader.go` around lines 715 - 723, The code currently hides dialer initialization failures by returning an empty []*templates.Template{} when protocolstate.Init(typesOpts) fails or when protocolstate.GetDialersWithId(typesOpts.ExecutionId) returns nil; instead change the loader function to return an error (e.g., change signature from func ...() []*templates.Template to func ...() ([]*templates.Template, error)), and replace the two returns of an empty slice with returns that wrap the underlying error (include the protocolstate.Init error and a descriptive error when GetDialersWithId returns nil referencing typesOpts.ExecutionId). Update all callers to handle the (templates, error) result and propagate or handle errors appropriately; use store.logger.Error().Err(err).Msgf(...) to keep logging consistent while returning the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 726-728: The code currently panics when syncutil.New(...) fails;
replace the panic in the wgLoadTemplates creation (wgLoadTemplates, errWg :=
syncutil.New(...)) with graceful error handling: log the error using the
package/logger already used in this file (same style as the surrounding error
handling) and return from the function with that error (or a wrapped error) so
the loader fails gracefully instead of panicking; ensure you reference
wgLoadTemplates and errWg, check errWg != nil, call the existing logger to
record the failure, and return the error to the caller.
---
Duplicate comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 715-723: The code currently hides dialer initialization failures
by returning an empty []*templates.Template{} when protocolstate.Init(typesOpts)
fails or when protocolstate.GetDialersWithId(typesOpts.ExecutionId) returns nil;
instead change the loader function to return an error (e.g., change signature
from func ...() []*templates.Template to func ...() ([]*templates.Template,
error)), and replace the two returns of an empty slice with returns that wrap
the underlying error (include the protocolstate.Init error and a descriptive
error when GetDialersWithId returns nil referencing typesOpts.ExecutionId).
Update all callers to handle the (templates, error) result and propagate or
handle errors appropriately; use store.logger.Error().Err(err).Msgf(...) to keep
logging consistent while returning the error.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/catalog/loader/loader.go (1)
713-724:dialersis assigned twice but never consumed — re-introduced dead assignment.Both assignments on lines 713 and 719 exist solely for nil-checks. The variable is never passed downstream. Inline the checks to avoid the dead assignment, as was suggested in the prior review round.
Proposed fix
- dialers := protocolstate.GetDialersWithId(typesOpts.ExecutionId) - if dialers == nil { + if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { if err := protocolstate.Init(typesOpts); err != nil { store.logger.Error().Msgf("dialers init failed for executionId %s: %s", typesOpts.ExecutionId, err) return []*templates.Template{} } - dialers = protocolstate.GetDialersWithId(typesOpts.ExecutionId) - if dialers == nil { + if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { store.logger.Error().Msgf("dialers with executionId %s not found", typesOpts.ExecutionId) return []*templates.Template{} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader.go` around lines 713 - 724, The code currently assigns dialers twice via protocolstate.GetDialersWithId(typesOpts.ExecutionId) but never uses the variable, creating a dead assignment; replace those assignments with inline nil checks: call protocolstate.GetDialersWithId(typesOpts.ExecutionId) directly in the if conditions (e.g., if protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { ... }) and keep the existing protocolstate.Init(typesOpts) and store.logger.Error() branches unchanged, removing the unused dialers variable entirely so there are no redundant assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/catalog/loader/loader.go`:
- Around line 713-724: The code currently assigns dialers twice via
protocolstate.GetDialersWithId(typesOpts.ExecutionId) but never uses the
variable, creating a dead assignment; replace those assignments with inline nil
checks: call protocolstate.GetDialersWithId(typesOpts.ExecutionId) directly in
the if conditions (e.g., if
protocolstate.GetDialersWithId(typesOpts.ExecutionId) == nil { ... }) and keep
the existing protocolstate.Init(typesOpts) and store.logger.Error() branches
unchanged, removing the unused dialers variable entirely so there are no
redundant assignments.
Author
|
Addressed the remaining panic path too: if wait-group creation fails, we now log the error and return an empty slice instead of panicking.\n\nLocal check: |
Author
|
(Note: the earlier 'go: cannot find main module' output was from the local shell cwd; comment posted successfully.) |
Author
|
Re: error propagation vs empty slice Agree that returning an error would be more programmatically explicit, but I kept the existing Current behavior is: no panic; we log the failure (with executionId context) and return an empty slice. This mirrors other loader paths that log-and-continue. If maintainers prefer the signature change, I can follow up with a separate PR that updates the signature + callers accordingly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6674.
When dialers are missing for the current executionId, the loader currently panics and crashes the process.
This changes the behavior to:
Local check:
/claim #6674
Summary by CodeRabbit