ISSUE-761: add dead hosts & add todos#1695
Open
whereMyCleanCode wants to merge 1 commit into
Open
Conversation
WalkthroughThis PR implements dead host tracking across the scanning pipeline: hosts that are targeted but do not respond are identified during ICMP scanning, aggregated in result collection, compared against all expected targets by the runner, and finally serialized in output with an ChangesDead host tracking and reporting
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/result/results.go (1)
128-170:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
GetIPsPortsreleases the lock before the goroutine consumes shared maps
Line 129acquiresRLock, butLine 130defers unlock in the parent function. Once the function returns, the goroutine keeps iteratingr.ipPorts/r.deadHostswithout protection, which can panic under concurrent writes.Suggested fix (snapshot under lock, then iterate unlocked)
func (r *Result) GetIPsPorts() chan *HostResult { - r.RLock() - defer r.RUnlock() - - out := make(chan *HostResult) - - go func() { + r.RLock() + ipPortsSnapshot := make(map[string][]*port.Port, len(r.ipPorts)) + skippedSnapshot := make(map[string]struct{}, len(r.skipped)) + deadHostsSnapshot := append([]string(nil), r.deadHosts...) + for ip, ports := range r.ipPorts { + ipPortsSnapshot[ip] = maps.Values(ports) + } + for ip := range r.skipped { + skippedSnapshot[ip] = struct{}{} + } + r.RUnlock() + + out := make(chan *HostResult) + go func() { defer close(out) - - // Живые хосты с портами - for ip, ports := range r.ipPorts { + for ip, ports := range ipPortsSnapshot { confidenceLevel := confidence.Normal - if r.HasSkipped(ip) { + if _, ok := skippedSnapshot[ip]; ok { confidenceLevel = confidence.Low } - hostResult := &HostResult{ IP: ip, - Ports: maps.Values(ports), + Ports: ports, Confidence: confidenceLevel, IsDeadHost: false, } @@ - for _, ip := range r.deadHosts { + for _, ip := range deadHostsSnapshot { out <- &HostResult{ IP: ip, IsDeadHost: true, } } }() return out }🤖 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 `@pkg/result/results.go` around lines 128 - 170, GetIPsPorts holds r.RLock()/defer RUnlock() but starts a goroutine that reads r.ipPorts and r.deadHosts after the function returns, allowing concurrent mutation; fix by snapshotting the shared structures while under lock (copy r.ipPorts into a local map/structure and r.deadHosts into a local slice) inside GetIPsPorts, then release the lock and have the goroutine iterate over those local copies when building HostResult (preserve logic for isPrivateIP/GetMacAddress and confidence handling), so the goroutine no longer accesses r.ipPorts or r.deadHosts after unlock.
🤖 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 `@internal/pdcp/writer.go`:
- Around line 95-98: The code currently substitutes nil hr with
&result.HostResult{} and sends it into u.data, which creates synthetic empty
results; change the logic to skip sending when hr == nil (do not replace with an
empty result) and only enqueue when hr != nil (u.data <- *hr); optionally emit a
warning/log when hr is nil (use the existing logger if available) instead of
serializing an empty result to avoid inflating counts.
In `@pkg/result/results.go`:
- Around line 97-101: AddDeadHost currently appends IPs blindly to r.deadHosts
causing duplicates; change it to avoid duplicates by either (A) checking the
existing slice for the ip before appending inside Result.AddDeadHost or (B)
convert the Result.deadHosts storage to a map[string]struct{} (e.g.,
deadHostsSet) and update AddDeadHost to set the map key; also update any
consumers such as HasDeadHosts and any count/reporting logic to read from the
de-duplicated set (or derive counts from the unique map keys) and keep locking
(r.Lock()/defer r.Unlock()) around the check/update.
In `@pkg/runner/runner.go`:
- Around line 1324-1328: The dead-host printing block is incorrectly placed
inside the per-host output loop and checks scanResults.HasDeadHosts(), causing
either no output or repeated prints; move the block that iterates
r.scanner.HostDiscoveryResults.GetDeadHosts() out of the per-host loop (i.e.,
run it once after handleOutput/after processing all hosts) and base its
execution on HostDiscoveryResults (not the per-host scanResults) so dead hosts
are logged exactly once; update the logic around scanResults.HasDeadHosts(),
handleOutput, and r.scanner.HostDiscoveryResults.GetDeadHosts() accordingly.
- Around line 1052-1066: The dead-host calculation is using the wrong set
checks: in the Hosts.Scan loop change the filter so you add only valid IP/CIDR
targets (use iputil.ToCidr(string(ip)) != nil) into allTargets instead of
treating ToCidr() == nil as the target; and when collecting alive hosts read
from the host-discovery IP set (use r.scanner.HostDiscoveryResults.GetIPs() or
the method that corresponds to AddIp) rather than GetIPsPorts() (which
corresponds to AddPort) so live IPs added via AddIp are detected. Ensure you
update references to r.scanner.IPRanger.Hosts.Scan, iputil.ToCidr, and
r.scanner.HostDiscoveryResults.GetIPsPorts to the correct checks/methods.
In `@pkg/scan/ndp.go`:
- Around line 18-30: The code reassigns destAddr after setting destAddr.Zone,
discarding the IPv6 zone/scope-id needed for link-local NDP; instead of
replacing destAddr with a new &net.UDPAddr{IP: ...}, assign the parsed IP into
the existing destAddr (e.g., destAddr.IP = net.ParseIP(ip)) so the
previously-set destAddr.Zone (from iNetwork.Name via PkgRouter.Route / iNetwork)
is preserved; keep the existing error handling around iNetwork and err as-is.
In `@pkg/scan/scan_raw.go`:
- Around line 33-37: The new ICMP constants icmpHeaderLen and icmpLen are
currently unused and failing golangci-lint; either delete these constants or
actually use them where ICMP packet lengths are handled (e.g., replace
hard-coded 4/84 literals in any ICMP read/parse logic in pkg/scan/scan_raw.go or
the TODO stub that reads ICMP request byte arrays) so the symbols icmpHeaderLen
and icmpLen are referenced; ensure any added usage consistently replaces magic
numbers and update tests/comments accordingly.
---
Outside diff comments:
In `@pkg/result/results.go`:
- Around line 128-170: GetIPsPorts holds r.RLock()/defer RUnlock() but starts a
goroutine that reads r.ipPorts and r.deadHosts after the function returns,
allowing concurrent mutation; fix by snapshotting the shared structures while
under lock (copy r.ipPorts into a local map/structure and r.deadHosts into a
local slice) inside GetIPsPorts, then release the lock and have the goroutine
iterate over those local copies when building HostResult (preserve logic for
isPrivateIP/GetMacAddress and confidence handling), so the goroutine no longer
accesses r.ipPorts or r.deadHosts after unlock.
🪄 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
Run ID: 3ebb5dc4-9d4f-4412-8646-662ce189447e
📒 Files selected for processing (11)
.gitignorecmd/naabu/main.gointernal/pdcp/writer.gopkg/fingerprint/parser.gopkg/result/results.gopkg/runner/output.gopkg/runner/runner.gopkg/runner/targets.gopkg/scan/ndp.gopkg/scan/scan.gopkg/scan/scan_raw.go
Comment on lines
+95
to
98
| if hr == nil { | ||
| hr = &result.HostResult{} | ||
| } | ||
| u.data <- *hr |
There was a problem hiding this comment.
Avoid enqueueing synthetic empty results for nil callbacks.
At Line 95, replacing nil with &result.HostResult{} and sending it at Line 98 can upload blank records and inflate result counts. Nil callbacks should be ignored (or warned), not serialized as data.
Proposed fix
func (u *UploadWriter) GetWriterCallback() result.ResultFn {
return func(hr *result.HostResult) {
- if hr == nil {
- hr = &result.HostResult{}
- }
+ if hr == nil {
+ return
+ }
u.data <- *hr
}
}📝 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.
Suggested change
| if hr == nil { | |
| hr = &result.HostResult{} | |
| } | |
| u.data <- *hr | |
| func (u *UploadWriter) GetWriterCallback() result.ResultFn { | |
| return func(hr *result.HostResult) { | |
| if hr == nil { | |
| return | |
| } | |
| u.data <- *hr | |
| } | |
| } |
🤖 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 `@internal/pdcp/writer.go` around lines 95 - 98, The code currently substitutes
nil hr with &result.HostResult{} and sends it into u.data, which creates
synthetic empty results; change the logic to skip sending when hr == nil (do not
replace with an empty result) and only enqueue when hr != nil (u.data <- *hr);
optionally emit a warning/log when hr is nil (use the existing logger if
available) instead of serializing an empty result to avoid inflating counts.
Comment on lines
+97
to
+101
| func (r *Result) AddDeadHost(ip string) { | ||
| r.Lock() | ||
| defer r.Unlock() | ||
| r.deadHosts = append(r.deadHosts, ip) | ||
| } |
There was a problem hiding this comment.
AddDeadHost allows duplicate entries, skewing counts/output
Line 100 appends blindly. The same IP can be marked dead multiple times (e.g., multiple probes + post-discovery reconciliation), which inflates HasDeadHosts/count-based reporting and repeats output.
🤖 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 `@pkg/result/results.go` around lines 97 - 101, AddDeadHost currently appends
IPs blindly to r.deadHosts causing duplicates; change it to avoid duplicates by
either (A) checking the existing slice for the ip before appending inside
Result.AddDeadHost or (B) convert the Result.deadHosts storage to a
map[string]struct{} (e.g., deadHostsSet) and update AddDeadHost to set the map
key; also update any consumers such as HasDeadHosts and any count/reporting
logic to read from the de-duplicated set (or derive counts from the unique map
keys) and keep locking (r.Lock()/defer r.Unlock()) around the check/update.
Comment on lines
+1052
to
+1066
| r.scanner.IPRanger.Hosts.Scan(func(ip, _ []byte) error { | ||
| // Игнорируем ip:port комбинации | ||
| if cidr := iputil.ToCidr(string(ip)); cidr == nil { | ||
| allTargets[string(ip)] = true | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| // Получаем ответившие хосты | ||
| aliveHosts := make(map[string]bool) | ||
| for hostResult := range r.scanner.HostDiscoveryResults.GetIPsPorts() { | ||
| if !hostResult.IsDeadHost { | ||
| aliveHosts[hostResult.IP] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
calculateDeadHosts currently compares the wrong sets
Two logic errors in this block break dead-host classification:
Line 1054:iputil.ToCidr(...) == nilcaptures non-CIDR/non-IP entries (oftenip:port), not the normal IP/CIDR targets.Line 1062: alive hosts are read fromHostDiscoveryResults.GetIPsPorts(), but host discovery stores liveness viaAddIp, notAddPort, so this can miss alive hosts entirely.
Result: hosts can be incorrectly marked dead en masse.
🤖 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 `@pkg/runner/runner.go` around lines 1052 - 1066, The dead-host calculation is
using the wrong set checks: in the Hosts.Scan loop change the filter so you add
only valid IP/CIDR targets (use iputil.ToCidr(string(ip)) != nil) into
allTargets instead of treating ToCidr() == nil as the target; and when
collecting alive hosts read from the host-discovery IP set (use
r.scanner.HostDiscoveryResults.GetIPs() or the method that corresponds to AddIp)
rather than GetIPsPorts() (which corresponds to AddPort) so live IPs added via
AddIp are detected. Ensure you update references to
r.scanner.IPRanger.Hosts.Scan, iputil.ToCidr, and
r.scanner.HostDiscoveryResults.GetIPsPorts to the correct checks/methods.
Comment on lines
+1324
to
+1328
| if scanResults.HasDeadHosts() { | ||
| for _, deadHost := range r.scanner.HostDiscoveryResults.GetDeadHosts() { | ||
| gologger.Info().Msgf("Dead host %s\n", deadHost) | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead-host output is gated/placed incorrectly
This block is inside the per-host output loop and checks scanResults.HasDeadHosts() while reading from r.scanner.HostDiscoveryResults. In the normal path (handleOutput(r.scanner.ScanResults)), that condition is usually false, so dead hosts are not reported; if true, the same dead-host list is printed repeatedly.
🤖 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 `@pkg/runner/runner.go` around lines 1324 - 1328, The dead-host printing block
is incorrectly placed inside the per-host output loop and checks
scanResults.HasDeadHosts(), causing either no output or repeated prints; move
the block that iterates r.scanner.HostDiscoveryResults.GetDeadHosts() out of the
per-host loop (i.e., run it once after handleOutput/after processing all hosts)
and base its execution on HostDiscoveryResults (not the per-host scanResults) so
dead hosts are logged exactly once; update the logic around
scanResults.HasDeadHosts(), handleOutput, and
r.scanner.HostDiscoveryResults.GetDeadHosts() accordingly.
Comment on lines
+18
to
+30
| iNetwork, _, _, err := PkgRouter.Route(net.ParseIP(ip)) | ||
| destAddr := &net.UDPAddr{} | ||
| if iNetwork == nil { | ||
| err = errors.New("Could not send PingNdp Request packet to " + ip + ": no interface with outbound source found") | ||
| } else { | ||
| destAddr.Zone = iNetwork.Name | ||
| } | ||
|
|
||
| if err != nil { | ||
| gologger.Debug().Msgf("%s\n", err) | ||
| return | ||
| } | ||
| destAddr := &net.UDPAddr{IP: net.ParseIP(ip), Zone: networkInterface.Name} | ||
| destAddr = &net.UDPAddr{IP: net.ParseIP(ip)} |
There was a problem hiding this comment.
IPv6 Zone is discarded by reassignment, breaking link-local NDP.
Line 23 sets destAddr.Zone = iNetwork.Name, but line 30 immediately replaces destAddr with a new &net.UDPAddr{IP: ...} that has no Zone. This makes the zone-setting code dead and will cause NDP packets to fail on IPv6 link-local addresses where the zone/scope-id is required to identify the outgoing interface.
🐛 Proposed fix: set IP field instead of reassigning struct
if err != nil {
gologger.Debug().Msgf("%s\n", err)
return
}
- destAddr = &net.UDPAddr{IP: net.ParseIP(ip)}
+ destAddr.IP = net.ParseIP(ip)
m := icmp.Message{📝 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.
Suggested change
| iNetwork, _, _, err := PkgRouter.Route(net.ParseIP(ip)) | |
| destAddr := &net.UDPAddr{} | |
| if iNetwork == nil { | |
| err = errors.New("Could not send PingNdp Request packet to " + ip + ": no interface with outbound source found") | |
| } else { | |
| destAddr.Zone = iNetwork.Name | |
| } | |
| if err != nil { | |
| gologger.Debug().Msgf("%s\n", err) | |
| return | |
| } | |
| destAddr := &net.UDPAddr{IP: net.ParseIP(ip), Zone: networkInterface.Name} | |
| destAddr = &net.UDPAddr{IP: net.ParseIP(ip)} | |
| iNetwork, _, _, err := PkgRouter.Route(net.ParseIP(ip)) | |
| destAddr := &net.UDPAddr{} | |
| if iNetwork == nil { | |
| err = errors.New("Could not send PingNdp Request packet to " + ip + ": no interface with outbound source found") | |
| } else { | |
| destAddr.Zone = iNetwork.Name | |
| } | |
| if err != nil { | |
| gologger.Debug().Msgf("%s\n", err) | |
| return | |
| } | |
| destAddr.IP = net.ParseIP(ip) |
🤖 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 `@pkg/scan/ndp.go` around lines 18 - 30, The code reassigns destAddr after
setting destAddr.Zone, discarding the IPv6 zone/scope-id needed for link-local
NDP; instead of replacing destAddr with a new &net.UDPAddr{IP: ...}, assign the
parsed IP into the existing destAddr (e.g., destAddr.IP = net.ParseIP(ip)) so
the previously-set destAddr.Zone (from iNetwork.Name via PkgRouter.Route /
iNetwork) is preserved; keep the existing error handling around iNetwork and err
as-is.
Comment on lines
+33
to
+37
| // TODO: use this for reade byte array from ICMP req's | ||
| const ( | ||
| icmpHeaderLen = 4 | ||
| icmpLen = 84 | ||
| ) |
There was a problem hiding this comment.
Remove or use the new ICMP constants to keep lint green
Line 35 and Line 36 are currently unused (matches the provided golangci-lint errors). If CI gates on lint, this blocks merge.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 35-35: const icmpHeaderLen is unused
(unused)
[error] 36-36: const icmpLen is unused
(unused)
🤖 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 `@pkg/scan/scan_raw.go` around lines 33 - 37, The new ICMP constants
icmpHeaderLen and icmpLen are currently unused and failing golangci-lint; either
delete these constants or actually use them where ICMP packet lengths are
handled (e.g., replace hard-coded 4/84 literals in any ICMP read/parse logic in
pkg/scan/scan_raw.go or the TODO stub that reads ICMP request byte arrays) so
the symbols icmpHeaderLen and icmpLen are referenced; ensure any added usage
consistently replaces magic numbers and update tests/comments 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.
Summary by CodeRabbit
New Features
IsDeadHostflag for each host.Bug Fixes