Skip to content

Implement full context support in ZDNS and better SIGTERM/SIGINT handling #538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 15, 2025

Conversation

phillip-stephens
Copy link
Contributor

@phillip-stephens phillip-stephens commented Apr 10, 2025

On Mac, there's always been an annoying behavior (existing back to at least v1.1.0 that I saw) where SIGINT (ctrl + C) wouldn't kill ZDNS. I've had ZDNS continue after SIGINT for multiple minutes, and I end up just closing the terminal and opening another. Linux doesn't seem to have this behavior and ZDNS will exit immediately.

This PR implements both:

  • Better handling of context.Context's in ZDNS. Now the main API's (ExternalLookup(), etc.) all accept a ctx object.
  • Better handling of SIGINT when using ZDNS as a CLI tool. Now, a goroutine will watch for SIGINT and SIGTERM and cancel the main context.Context. This ensures ZDNS exits quickly on Mac.

Limitation

2 Second Delay in Cancellation

With this PR, ZDNS on Mac continues for 2-3 seconds post SIGINT. Using stack dumps, I noticed all goroutines are stuck in a zmap/dns lookup. At first this was really surprising since all the dns lookups we do use a context object which should be canceled when the main context is. However, looking closer at the source code for zmap/dns, only the ctx.Deadline is used to configure timeouts on the DNS connection object, and our default --network-timeout is 2s. So we can't cancel in-progress DNS requests and will have to let them expire. Regardless, this seems strictly better than our current state.

Final Status Update

Currently, the status handler will print a final Scan Complete; X names scanned... with SIGINT. Wasn't sure if it'd be better to just immediately quit or print this.

Update - Per PR discussion, we now print a Scan Aborted instead of Scan Complete to indicate to the user we aborted due to their signal and didn't exit cleanly.

Testing

v1.1.0 Behavior

Screen.Recording.2025-04-10.at.4.34.02.PM.mov

This PR Behavior

Screen.Recording.2025-04-15.at.2.27.19.PM.mov

@phillip-stephens phillip-stephens marked this pull request as ready for review April 10, 2025 23:39
@phillip-stephens phillip-stephens requested a review from a team as a code owner April 10, 2025 23:39
@phillip-stephens
Copy link
Contributor Author

@zakird Wondering if you have thoughts on the Final Status Update Limitation I listed in the description? Otherwise this is good to go.

@zakird
Copy link
Member

zakird commented Apr 11, 2025

Currently, the status handler will print a final Scan Complete; X names scanned... with SIGINT. Wasn't sure if it'd be better to just immediately quit or print this.

I don't think it super matters. If someone killed the process, they shouldn't really rely on the results. I'm fine leaving it has having it print the status message, but I thought that you might want to print something afterwards that indicates that the process has been interrupted so that the user doesn't inadvertently think that the process completed successfully.

@phillip-stephens phillip-stephens merged commit 1af9b42 into main Apr 15, 2025
3 checks passed
@phillip-stephens phillip-stephens deleted the phillip/fix-hard-to-kill branch April 15, 2025 21:36
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.

2 participants