Error handling difficult and not reliable

[noneymous]

Issue

It is currently difficult to check whether an Nmap scan failed, due to two reasons:

• Error messages are spread across three locations (returned by Run(), result.NmapErrors and result.Stats.Finished.ErrorMsg)
• Error messages might actually be warnings. Nmap can only write warnings to stderr where we undestand them as errors

Examples generated by Run():

• "unable to parse nmap output"
• ErrScanTimeout

Examples returned via stderr:

• "Error resolving name" (target could not be resolved, hence not scanned)
• "No targets were specified"

Example found in result.Stats.Finished.ErrorMsg:

• "Error resolving name" (issue with exclude list)

Suggestion

I'm suggesting to change the signature of the Run() method to:
func (s *Scanner) Run() (*Run, []strings, []error) { }

This is a breaking change on purpose. It will prevent people from compiling their old code with the updated package. We do not want to affect their application behaviour without them noticing. Ideally, they should reconsider their error handling too.

With this suggestion we can return an intuitive set of result data: result, list of warnings and list of errors.

=> Result may exist as soon as the XML could be parsed (even if there are errors returned in parallel)
=> Warnings may be there and can be used if desired.
=> The list of errors can aggregate the error messages from our run method, stderr and a copy of those from the xml output.

Possible Implementation

• in case of timeout, just return the timeout error and nothing else. otherwise:
• take stderr and split it into errors and warnings to be returned subsequently
• parse xml. if parsing failed, add this error to stderr errors and return them all together. otherwise:
• search parsed xml for error messages and add them to the existing list of errors
• (drop result.NmapErrors again)

Really really nice to have

I'ts tricky, but mybe we want to introdcue (more) error constants (in errors.go) for whatever we run across. So that other developers directly are aware of them and can handle them as they need.

[noneymous]

Sample code to split errors/warnings from stderr:

func Test_parseError(t *testing.T) {

	err := bytes.NewBufferString(`WARNING: Service xxx.xxx.xxx.xxx:7100 had already soft-matched rtsp, but now soft-matched sip; ignoring second value
WARNING: Service yyy.yyy.yyy.yyy:47000 had already soft-matched rtsp, but now soft-matched sip; ignoring second value
Trying to delete NSI, but could not find 1 of the purportedly pending events on that IOD.

Trying to delete NSI, but could not find 1 of the purportedly pending events on that IOD.

`)

	var nmapErrors []string
	var nmapWarnings []string

	// Investigate stderr output
	errStr := err.String()
	if len(err.String()) > 0 {

		// Prepare regex
		r := regexp.MustCompile("(?i)^Warning: .*")

		// Iterate error output
		for _, line := range strings.Split(errStr, "\n"){
			if len(line) > 0 {
				match := r.FindString(line)
				if match == "" {
					nmapErrors = append(nmapErrors, line)
				} else {
					nmapWarnings = append(nmapWarnings, line)
				}
			}
		}
	}

	fmt.Println(nmapErrors)
	fmt.Println(nmapWarnings)
}

[noneymous]

It MIGHT be that we discover some warnings which we cannot clearly identify as warnings. I think the suggested solution still is the best one, because it also gives a lot of flexibility. People can decide to ignore certain errors or even all of them. If the XML parses, they can use it. Furthermore, I think we need to take it step by step and improve the solution incrementally.

[noneymous]

Here the python nmap package for reference. Although, I think this is not the best code.
https://bitbucket.org/xael/python-nmap/src/default/nmap/nmap.py

[noneymous]

After looking more into it, I'm slowly coming to the conclusion that anything on stderr should be undestood as a warning and everyboody should check these "warnings" for things they care about. If something is critical, Nmap returns valid XML but with the attribute status="error" and an error message in result.Stats.Finished.ErrorMsg.

E.g. Nmap may write "Error resolving name" to stderr. It can either be a warning or a critical message. It is a warning if a scan target (hostname) could not be resolved. It is critical if it's a host from the exclude file that could not be resolved. In both cases Nmap will write the same message to stderr. But only in the second case the Nmap XML output contains status=error and an error message. Additionally the message "QUITTING" can be found on stderr.