Fix improper signal handling and add proper context cancellation#1461
Fix improper signal handling and add proper context cancellation#1461Mzack9999 merged 9 commits intoprojectdiscovery:devfrom Mehran-Seifalinia:dev
Conversation
Used p.Protocol.String() instead of %d to ensure proper string representation
- Replaced usage of context.TODO() with context.WithCancel() to allow graceful shutdowns when receiving interrupt signals. - Updated signal handling to read a single signal event instead of looping indefinitely. - Added safety checks before accessing runner fields during shutdown. - Improved the shutdown flow to ensure better resource cleanup and stability on termination.
WalkthroughThe main function in the application has been updated to use a cancellable context for managing the enumeration process. Signal handling has been improved to listen for both interrupt and termination signals. When such a signal is received, the application logs the event, cancels ongoing operations, attempts to save the resume configuration if it exists, displays scan results, and performs cleanup. The handling of the resume configuration and runner closure is now conditional on their existence. The signal handling loop has been simplified to a single receive operation. Additionally, deferred cleanup operations across various commands and packages now include error handling and logging instead of silently ignoring errors. Several close operations on network connections and files have been updated to explicitly ignore errors or log them, improving resource cleanup robustness. Some switch statements were refactored for clarity without changing functionality. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/naabu/main.go (1)
53-56: Consider implementing logger flush mechanism.The placeholder comment suggests a potential improvement that hasn't been implemented yet.
Either implement the gologger flush/close functionality or remove the placeholder comment to avoid confusion. If it's a planned future enhancement, consider adding a proper TODO comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/naabu/main.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/naabu/main.go (1)
pkg/runner/resume.go (2)
ResumeCfg(32-37)DefaultResumeFilePath(27-29)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-windows
- GitHub Check: build-mac
- GitHub Check: release-test-linux
- GitHub Check: build-linux
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: release-test-windows
🔇 Additional comments (9)
cmd/naabu/main.go (9)
7-7: Appropriate import addition for signal handling.Adding
syscallimport is necessary to support theSIGTERMsignal handling that's been implemented.
23-25: Good implementation of cancellable context.Using a cancellable context is essential for proper resource management. This change allows long-running operations to be gracefully terminated when the user interrupts the scan.
28-28: Enhanced signal handling to include SIGTERM.Adding
syscall.SIGTERMalongsideos.Interruptimproves shutdown handling by responding to both interactive interrupts (Ctrl+C) and system termination requests.
30-32: Improved signal reception and logging.The change from a
for rangeloop to a single signal receive prevents multiple executions of shutdown routines. Adding signal type logging is helpful for debugging.
33-35: Proper context cancellation handling.Calling
cancel()on signal receipt properly propagates termination signals to all goroutines using the context, allowing them to clean up and exit gracefully.
37-42: Conditional resume configuration saving.The nil check before accessing
options.ResumeCfgprevents potential nil pointer dereferences. Good practice to log both the success message and error cases.
44-47: Conditional scan result display.The nil check before accessing
naabuRunnerprevents potential panics if the runner wasn't properly initialized.
59-60: Correctly passing cancellable context to enumeration.Using the cancellable context when running the enumeration ensures that the scan can be properly interrupted when a termination signal is received.
64-67: Proper cleanup of resume configuration.The nil check before cleaning up the resume configuration prevents potential nil pointer dereferences.
|
Thanks for your contribution @Mehran-Seifalinia ! :) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (15)
pkg/runner/healthcheck.go(2 hunks)pkg/runner/ips_test.go(2 hunks)pkg/runner/output.go(1 hunks)pkg/runner/resume.go(1 hunks)pkg/runner/runner.go(1 hunks)pkg/runner/runner_test.go(5 hunks)pkg/runner/targets.go(4 hunks)pkg/runner/targets_test.go(2 hunks)pkg/scan/connect.go(1 hunks)pkg/scan/connect_test.go(2 hunks)pkg/scan/externalip.go(1 hunks)pkg/scan/icmp.go(2 hunks)pkg/scan/ping.go(1 hunks)pkg/scan/scan.go(1 hunks)pkg/scan/scan_unix.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/scan/connect.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/scan/scan_unix.go (2)
pkg/scan/scan.go (7)
IcmpEchoRequest(70-70)IcmpTimestampRequest(71-71)IcmpAddressMaskRequest(72-72)Ndp(74-74)Arp(73-73)Syn(68-68)Ack(69-69)pkg/scan/icmp.go (3)
PingIcmpEchoRequestAsync(79-127)PingIcmpTimestampRequestAsync(180-207)PingIcmpAddressMaskRequestAsync(270-304)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-windows
- GitHub Check: build-mac
- GitHub Check: build-linux
- GitHub Check: Functional Test (ubuntu-latest)
🔇 Additional comments (30)
pkg/runner/ips_test.go (1)
32-36: Improved error handling for test file cleanup.The deferred function now properly logs any errors encountered during test file cleanup instead of silently failing. This is a good practice for test code as it helps with debugging potential resource cleanup issues.
pkg/scan/icmp.go (2)
37-39: Explicit error handling for connection closure.The deferred function now explicitly acknowledges and discards any errors from connection closure, making the error handling intention clear rather than implicitly ignoring errors.
136-138: Explicit error handling for connection closure.Similar to the earlier change, this deferred function now explicitly acknowledges and discards any errors from connection closure, making the error handling intention clear.
pkg/scan/externalip.go (1)
21-23: Explicit error handling for HTTP response body closure.The deferred function now explicitly acknowledges and discards any errors from response body closure, which is a good practice for HTTP client code. This change aligns with the PR's focus on improving resource cleanup.
pkg/scan/connect_test.go (2)
20-24: Improved error reporting for listener closure in tests.The deferred function now properly logs any errors encountered during listener closure, which improves test diagnostics and helps identify potential resource cleanup issues.
30-32: Explicit error handling for connection closure.The deferred function now explicitly acknowledges and discards any errors from connection closure, making the error handling intention clear rather than implicitly ignoring errors.
pkg/runner/healthcheck.go (2)
54-54: Improved error handling for TCP IPv4 connection closure.The change explicitly ignores the error returned from
Close(), making it clear that you're intentionally discarding the error rather than silently ignoring it.
63-63: Improved error handling for TCP IPv6 connection closure.The change explicitly ignores the error returned from
Close(), which follows best practices for making ignored errors visible.pkg/runner/targets_test.go (1)
4-18: Improved error handling during resource cleanup in tests.The change wraps the
ipranger.Close()call in a deferred anonymous function that logs any error instead of silently ignoring it. This enhances error visibility during testing and aligns with the PR's goal of improving resource management.pkg/runner/output.go (1)
121-121: Explicitly ignoring flush error when write already failed.This change makes it clear that you're intentionally ignoring the error from
bufwriter.Flush()when returning the original write error. This follows the pattern of explicit error handling seen throughout the PR.pkg/runner/runner.go (1)
978-982: Improved error handling during file closure.The change wraps the
file.Close()call in a deferred anonymous function that logs any error that might occur. This enhances error visibility during cleanup and aligns with the PR's goal of improving resource management and application stability.pkg/scan/ping.go (1)
50-52: Improved error handling pattern in resource cleanupThe code now explicitly ignores any errors returned from closing the ICMP packet listener. This follows a cleaner pattern for deferred resource cleanup and aligns with the PR's focus on proper resource management during application shutdown.
pkg/runner/resume.go (1)
86-86: Enhanced error handling clarityThe code now explicitly ignores any errors from the
os.Removeoperation by using the blank identifier. This makes the intention clear that errors during resume file cleanup are non-critical and intentionally discarded.pkg/scan/scan.go (1)
420-422: Improved resource cleanup patternThe code now properly handles connection closure in a deferred anonymous function, explicitly ignoring any errors. This change ensures that network connections are closed cleanly even when errors occur, which is crucial for proper resource cleanup during application shutdown.
pkg/runner/targets.go (5)
46-50: Enhanced error handling in file resource managementThe code now properly handles and logs errors during file closure in a deferred function. This is a good improvement for resource cleanup and error visibility.
55-55: Explicit error ignoring for file operationsThe code now explicitly ignores return values from
fmt.Fprintfusing blank identifiers. This clarifies that these values aren't needed for the application flow.
65-69: Improved file resource cleanup with error loggingAdded proper error handling and logging for the hosts file closure operation, which enhances visibility of any issues during resource cleanup.
85-85: Consistent error handling patternThe explicit ignoring of
fmt.Fprintfreturn values maintains consistency with the error handling pattern used earlier in the file.
101-105: Robust file handling with error loggingThe code now properly logs any errors that occur when closing the targets file, which improves error visibility and ensures consistent error handling throughout the application.
pkg/runner/runner_test.go (6)
6-6: Added dependency for error logging.The
logpackage is now imported to support the new error-logging code pattern during resource cleanup operations.
215-219: Improved error handling during cleanup.The direct
os.Removecall has been wrapped in a deferred anonymous function with proper error handling, ensuring any file removal errors are logged rather than silently ignored.
222-223: Added error checking for runner.Close().The code now properly captures and validates the error returned from
runner.Close(), improving error visibility during test cleanup.
736-737: Added error validation for runner.Close() in TestRunnerEnumeration.The code now properly captures and validates the error returned from
runner.Close()in the enumeration test, ensuring proper resource cleanup.
789-790: Added error validation for runner.Close() in TestRunnerHostDiscovery.Similar to other tests, the code now properly validates that closing the runner doesn't produce errors, ensuring thorough error checking during cleanup.
804-807: Improved resource cleanup in TestRunnerGetIPs with deferred error handling.The runner.Close() call is now wrapped in a deferred function with proper error validation, ensuring consistent error handling throughout test cleanup operations.
pkg/scan/scan_unix.go (5)
125-134: Refactored switch statement to use more idiomatic Go pattern.Changed from
switch { case pkg.flag == ... }to the more readable and idiomaticswitch pkg.flag { case ... }pattern, improving code clarity.
141-144: Refactored EthernetWriteWorker switch statement.Similar to the ICMPWriteWorker change, the switch statement has been refactored to use the idiomatic Go pattern of switching directly on the variable.
238-243: Improved switch statement in sendAsyncTCP4.Refactored to use the more idiomatic Go pattern for switch statements, making the code more consistent and readable.
337-342: Refactored switch statement in sendAsyncTCP6.Made consistent with other switch statement improvements, switching directly on the pkgFlag variable rather than using comparison in case statements.
871-873: Improved connection closure with explicit error handling.The connection Close() call is now wrapped in a deferred anonymous function that explicitly handles the error (ignoring it with _), aligning with better Go practices for resource cleanup.
| if err == nil && u4 != nil { | ||
| _ = u4.Close() |
There was a problem hiding this comment.
Fixed incorrect UDP connection check and closure.
This critical bug fix correctly checks the UDP connection variable u4 instead of the TCP connection variable c4, ensuring proper UDP resource cleanup.
| if err == nil && u6 != nil { | ||
| _ = u6.Close() |
There was a problem hiding this comment.
Fixed incorrect UDP connection check and closure.
This critical bug fix correctly checks the UDP connection variable u6 instead of the TCP connection variable c6, ensuring proper UDP resource cleanup.
Summary
This pull request fixes two critical issues related to shutdown handling in the
cmd/naabu/main.gofile:Context Management:
context.TODO()was used when running the enumeration. This prevented graceful cancellation when an interrupt (e.g., Ctrl+C) was received.context.WithCancel()is created, and its cancel function is triggered on interrupt signals, allowing any ongoing tasks to properly terminate.Signal Handling:
for rangeloop over the signal channel, which could trigger shutdown routines multiple times if multiple interrupts were received.<-c) to guarantee that shutdown operations are only performed once.Why These Changes Are Important
Close()were called multiple times.Potential Issues Prevented
Summary by CodeRabbit