DNS-OARC / ripeatlas

Go bindings for RIPE Atlas API
MIT License
11 stars 12 forks source link

Calling traceroute.Reply.Err() causes panic when err is not present #28

Closed jmeggitt closed 1 year ago

jmeggitt commented 1 year ago

Problem

Attempting to call the Err() method of traceroute.Reply will result in an unavoidable panic if the unmarshalled traceroute.Reply does not contain the err field.

Error

This error can be reproduced by unmarshalling and calling Err() on any reply without an err field.

panic: interface conversion: interface {} is nil, not string

goroutine 25 [running]:
github.com/DNS-OARC/ripeatlas/measurement/traceroute.(*Reply).Err(...)
        C:/Users/Jasper/go/pkg/mod/github.com/!d!n!s-!o!a!r!c/ripeatlas@v0.0.0-20221107153620-ac4f03274eaf/measurement/traceroute/reply.go:84

Cause

Following the changes make to traceroute.Reply.Err() in commit c11cc4c3e1, the err field of a traceroute hop reply was changed to an interface{} to accommodate both int and string fields. https://github.com/DNS-OARC/ripeatlas/blob/9ad3c83de8ba9920cb13a2a87f52737b21ccbfdf/measurement/traceroute/reply.go#L12

When this field is then read using the Err() method on traceroute.Reply it casts this field to a string.

https://github.com/DNS-OARC/ripeatlas/blob/9ad3c83de8ba9920cb13a2a87f52737b21ccbfdf/measurement/traceroute/reply.go#L64-L66

This leads to a panic since in the event that a reply is not present in the input JSON, UnmarshalJSON leaves the field as nil.

https://github.com/DNS-OARC/ripeatlas/blob/9ad3c83de8ba9920cb13a2a87f52737b21ccbfdf/measurement/traceroute/reply.go#L34-L44

Solution

In the case where Err is nil, initialize it to an empty string. This gives the Err() method the same output on non-erroring replies that it had prior to commit c11cc4c3e1 and maintains compatibility with existing use.

jelu commented 1 year ago

Thanks for the very detailed report. Please feel free to do a less descriptive pull-request with a fix in the future if you have :smile: