fix: ensure connection cleanup in JARM fingerprint loop - #2545
Conversation
Use defer to guarantee connection closure even when errors occur during probe processing, preventing resource leaks in long-running scans.
WalkthroughThe JARM fingerprint probe flow now scopes write, read, and parse operations in an inner function and defers connection closure, preserving existing error returns and successful result collection. ChangesJARM Probe Lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/hashes/jarm/jarmhash.go`:
- Line 54: Update the deferred connection cleanup in fingerprint to explicitly
discard the result of conn.Close, resolving the errcheck warning without
changing fingerprint’s error behavior.
- Around line 63-66: Update the conn.Read handling in the JARM probe flow to
capture the returned byte count and preserve partial data when an error
accompanies n > 0. Pass buff[:n] to jarm.ParseServerHello, and return
immediately only when no bytes were read; retain the existing error behavior
otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df0528f2-fb88-4bf0-9b4e-22af37ad17f7
📒 Files selected for processing (1)
common/hashes/jarm/jarmhash.go
| results = append(results, ans) | ||
|
|
||
| func() { | ||
| defer conn.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Handle the deferred Close error.
Line [54] triggers errcheck. Since fingerprint cannot propagate cleanup errors, explicitly discard the result or add a narrowly scoped lint exemption.
Proposed fix
- defer conn.Close()
+ defer func() {
+ _ = conn.Close()
+ }()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer conn.Close() | |
| defer func() { | |
| _ = conn.Close() | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 54-54: Error return value of conn.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/hashes/jarm/jarmhash.go` at line 54, Update the deferred connection
cleanup in fingerprint to explicitly discard the result of conn.Close, resolving
the errcheck warning without changing fingerprint’s error behavior.
Source: Linters/SAST tools
| if _, err = conn.Read(buff); err != nil { | ||
| return | ||
| } | ||
| ans, err := jarm.ParseServerHello(buff, probe) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'ParseServerHello|\.Read\(' common/hashes/jarm --glob '*.go'Repository: projectdiscovery/httpx
Length of output: 734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline common/hashes/jarm/jarmhash.go || true
echo
echo "== jarmhash.go relevant sections =="
sed -n '1,140p' common/hashes/jarm/jarmhash.go
echo
echo "== parse server hello implementation/usages =="
rg -n -C4 'func ParseServerHello|ParseServerHello\(' common --glob '*.go'
echo
echo "== any connection wrappers/readers in common/hashes/jarm =="
rg -n -C3 'Read\(|conn.*Read|Read\(' common/hashes/jarm --glob '*.go'Repository: projectdiscovery/httpx
Length of output: 3543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Acquire/OneTimePool definition and implementations =="
rg -n -C5 'func newOneTimePool|type .*OneTime|func \(.*\) Acquire|type .*Connection|type .*Conn' common --glob '*.go'
echo
echo "== imports and pool files =="
fd -e go . common/hashes/jarm -x sh -c 'echo "--- $1"; sed -n "1,160p" "$1"' sh {}Repository: projectdiscovery/httpx
Length of output: 6673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fastdialer Dial implementations and net.Conn wrappers =="
rg -n -C5 'func \(.*\) Dial|type .*Dial|Dial\(ctx|net.Conn|Read\(.*error' -g '*.go' .Repository: projectdiscovery/httpx
Length of output: 6826
🌐 Web query:
hdm jarm-go ParseServerHello buffer length expected
💡 Result:
In the context of the jarm-go library (widely used for JARM fingerprinting), the ParseServerHello function is responsible for parsing a TLS ServerHello message from raw byte data [1][2][3]. The error regarding "buffer length expected" or similar validation failures typically arises when the provided byte buffer is too short to contain a valid TLS ServerHello structure [4]. JARM probing relies on receiving a response from a server, which is then passed to the parsing logic [1][3]. Key technical context: 1. Expected Data: The function expects a byte array containing a valid TLS handshake response. If the connection received fewer bytes than required by the TLS protocol specification (or the specific implementation's parser), an error will be returned [4]. 2. JARM Context: When using jarm-go, the library typically receives up to 1484 bytes from the server [1][5]. If the network read is interrupted or the server sends an incomplete response, the buffer passed to ParseServerHello will not meet the minimum length requirements for header fields, resulting in a parsing error [1][4]. 3. Troubleshooting: If you are encountering this error, ensure that the data being passed into ParseServerHello is actually a complete TLS ServerHello handshake message. In many implementations, the data is expected to start with the TLS record header (e.g., 5 bytes for the record layer) followed by the handshake message [6][4]. If you are writing custom code or debugging, verify that your read operation has fully buffered the expected amount of data before attempting to parse it [4].
Citations:
- 1: https://github.com/hdm/jarm-go
- 2: https://pkg.go.dev/github.com/hdm/jarm-go
- 3: https://github.com/caffix/jarm-go
- 4: https://github.com/Nyarime/NRUP/blob/main/handshake.go
- 5: https://pkg.go.dev/github.com/RumbleDiscovery/jarm-go
- 6: https://github.com/go-gost/tls-dissector/blob/master/dissector.go
Preserve bytes returned with a read error.
conn.Read implements Go’s io.Reader contract, which allows n > 0 with a non-nil error. Returning immediately can discard a partial JARM response that ParseServerHello is still able to parse. Capture n and pass buff[:n] into ParseServerHello; only return when no bytes were read.
Suggested adjustment
- if _, err = conn.Read(buff); err != nil {
+ n, readErr := conn.Read(buff)
+ if n == 0 && readErr != nil {
return
}
- ans, err := jarm.ParseServerHello(buff, probe)
+ ans, err := jarm.ParseServerHello(buff[:n], probe)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err = conn.Read(buff); err != nil { | |
| return | |
| } | |
| ans, err := jarm.ParseServerHello(buff, probe) | |
| n, readErr := conn.Read(buff) | |
| if n == 0 && readErr != nil { | |
| return | |
| } | |
| ans, err := jarm.ParseServerHello(buff[:n], probe) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/hashes/jarm/jarmhash.go` around lines 63 - 66, Update the conn.Read
handling in the JARM probe flow to capture the returned byte count and preserve
partial data when an error accompanies n > 0. Pass buff[:n] to
jarm.ParseServerHello, and return immediately only when no bytes were read;
retain the existing error behavior otherwise.
1 participant
Summary
Fix JARM fingerprint connection leak in
common/hashes/jarm/jarmhash.go.Problem
In the
fingerprintfunction, the loop iterates over JARM probes and acquires connections from a pool. When errors occur duringWrite,Read, orParseServerHello, the function returns immediately without closing the connection, causing resource leaks. Even in the success path,conn.Close()is called manually which is error-prone if new exit paths are added.Fix
Wrap each probe iteration in an anonymous function with
defer conn.Close()to guarantee connection cleanup regardless of which error path is taken. This prevents resource exhaustion during long-running scans.Checklist
I apologize for any inconvenience and appreciate the maintainers' time reviewing this contribution.
Summary by CodeRabbit