Skip to content
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

Fix unable to parse nmap output for incomplete XML output #127

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

idkw
Copy link
Contributor

@idkw idkw commented Feb 21, 2024

This is a fix for #122

According to golang standard library documentation in https://pkg.go.dev/os/exec#Cmd.StdoutPipe

It is thus incorrect to call Wait before all reads from the pipe have completed

And in

nmap/nmap.go

Line 149 in a750324

err := cmd.Wait()
this is exactly what is done. Wait is called while io.Copy ([1] , [2]) is still performing reads on stdoutDuplicate.

The fix is inspired from a solution to a similar simplified problem

Before calling Wait we wait on a WaitGroup than is done when all reads on stdoutDuplicate have completed.

Finally I've added a test that runs a thousand nmap scans (takes about 5 seconds). The scans are done without a host so it returns almost immediately. You can verify the test fails when commenting out wg.Wait() in nmap.go on line 161. You can verify the test doesn't fail with that wg.Wait().

Feel free to remove the test from the MR if you think it's not worth to keep it once the fix is confirmed to be working.

Copy link
Owner

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

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

Thanks, nice catch :))

@Ullaakut Ullaakut merged commit 58d9339 into Ullaakut:master Feb 21, 2024
1 check failed
@idkw
Copy link
Contributor Author

idkw commented Feb 22, 2024

Thanks @Ullaakut for the quick review, do you think it can be released in a patch version ?

@idkw idkw deleted the fix-xml-output-parsing branch February 22, 2024 14:24
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