Skip to content

Fix(clientpool): remove global variable from clientpool in favour for local variable#7294

Merged
Mzack9999 merged 3 commits intoprojectdiscovery:devfrom
mikhail5555:fix/data-race-condition-global-variable
Mar 25, 2026
Merged

Fix(clientpool): remove global variable from clientpool in favour for local variable#7294
Mzack9999 merged 3 commits intoprojectdiscovery:devfrom
mikhail5555:fix/data-race-condition-global-variable

Conversation

@mikhail5555
Copy link
Copy Markdown
Contributor

@mikhail5555 mikhail5555 commented Mar 24, 2026

Proposed changes

When multiple instances of nuclei SDK are initialized, the clientpool.Init() function is called multiple times, leading to unexpected/overwrite behavior in the wrapperGet function.

Since the wrapperGet function already has access to the options, there is little reason to initialize this variable globally, and it can just be done in the wrapperGet func.

Flow changes

Based on this change, the flow also is slightly changed, before hand, it would only follow redirects if the option options.MaxRedirects > 0 (and thus ignore the FollowHostRedirects and FollowRedirects options if it was == 0.

Now if max(options.MaxRedirects, configuration.MaxRedirects) > 0 it will follow redirects (aka honoring the options).

Proof

  WARNING: DATA RACE
  Read at 0x00000ad7dbd8 by goroutine 863:
    github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool.wrappedGet()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool/clientpool.go:224 +0x3e4
    github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool.Get()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool/clientpool.go:180 +0x246
    github.com/projectdiscovery/nuclei/v3/pkg/protocols/http.(*Request).Compile()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/http.go:348 +0xc28
  
  Previous write at 0x00000ad7dbd8 by goroutine 846:
    github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool.Init()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/projectdiscovery/nuclei/v3/pkg/protocols/http/httpclientpool/clientpool.go:38 +0xb3
    github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/protocolinit.Init()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/protocolinit/init.go:23 +0x5b
    fmt.Sscanf()
        /opt/hostedtoolcache/go/1.25.0/x64/src/fmt/scan.go:114 +0x264
    github.com/syndtr/goleveldb/leveldb/storage.fsParseName()
        /home/runner/_work/<omitted>/<omitted>/vendor/github.com/syndtr/goleveldb/leveldb/storage/file_storage.go:657 +0x18e

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Summary by CodeRabbit

  • Refactor
    • Improved HTTP redirect handling: per-request redirect settings are now respected (including disabling redirects and per-request max redirects), removing shared mutable global state.
    • Removed the global initializer for the HTTP client pool from startup flow so the pool no longer requires a separate package-level init step.

@auto-assign auto-assign bot requested a review from dwisiswant0 March 24, 2026 08:22
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 71c44dd7-42e6-453d-9750-c0be425ccca7

📥 Commits

Reviewing files that changed from the base of the PR and between 49a0ffe and a8c8645.

📒 Files selected for processing (2)
  • pkg/protocols/common/protocolinit/init.go
  • pkg/protocols/http/httpclientpool/clientpool.go
💤 Files with no reviewable changes (1)
  • pkg/protocols/common/protocolinit/init.go

Walkthrough

Removed package-level mutable forceMaxRedirects and its Init initializer from the HTTP client pool; redirect handling now derives redirectFlow and maxRedirects from per-request options. Also removed the httpclientpool.Init(options) call from protocol initialization.

Changes

Cohort / File(s) Summary
HTTP client pool (redirect logic)
pkg/protocols/http/httpclientpool/clientpool.go
Removed package-level forceMaxRedirects and exported Init. wrappedGet no longer reads global state; it computes redirectFlow from options.ShouldFollowHTTPRedirects() and FollowHostRedirects, applies options.MaxRedirects only when > 0, and forces no-follow/maxRedirects=0 when options.DisableRedirects is set.
Protocol initialization
pkg/protocols/common/protocolinit/init.go
Removed import and call to httpclientpool.Init(options) from protocol initialization sequence; associated error handling branch removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nudged the globals out of sight,
Each request now hops by its own light.
Redirects follow what the options say,
No shared crumbs to lead astray. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing a global variable from clientpool in favor of a local variable to fix data race conditions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/protocols/http/httpclientpool/clientpool.go`:
- Around line 216-225: The code unconditionally overwrites the request/template
redirect cap by assigning maxRedirects = options.MaxRedirects even when
options.MaxRedirects is the unset/default (0). Change the assignment so
maxRedirects is only replaced when options.MaxRedirects is set (non-zero/greater
than 0), preserving the existing configuration.MaxRedirects/request-level cap
otherwise; keep the redirectFlow logic the same and let checkMaxRedirects()
continue to enforce the final cap.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c047238b-f46a-42da-a0a8-ef4019ca6975

📥 Commits

Reviewing files that changed from the base of the PR and between 406ad1a and 9cc8456.

📒 Files selected for processing (1)
  • pkg/protocols/http/httpclientpool/clientpool.go

@neo-by-projectdiscovery-dev
Copy link
Copy Markdown

neo-by-projectdiscovery-dev bot commented Mar 24, 2026

Neo - PR Security Review

No security issues found

Highlights

  • Removed httpclientpool.Init() function and global variable initialization
  • Removed call to httpclientpool.Init() from protocolinit package
  • Changes consist purely of code deletion - reducing attack surface

Comment @pdneo help for available commands. · Open in Neo

@mikhail5555 mikhail5555 force-pushed the fix/data-race-condition-global-variable branch from f0d5eff to 49a0ffe Compare March 24, 2026 08:33
@dwisiswant0 dwisiswant0 requested a review from Mzack9999 March 24, 2026 10:39
@Mzack9999 Mzack9999 merged commit 9e2366f into projectdiscovery:dev Mar 25, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants