Feat/ping for wireguard #35
Conversation
this is needed so that we can pass vpn.Dialer to HTTPConfig as part of the urlgetter experiment in OONI probe-cli.
|
please note that only the latest commit addresses the wireguard modifications. this branch piles up commits from the security fixes, and is needed for compat reasons, but this will be rebased after the other PRs are merged. |
| if err != nil { | ||
| fmt.Println("ERROR", err) |
There was a problem hiding this comment.
Please, do not use fmt.Println to print errors. If there is a desire to log errors, we should have a customizable logger such that we can correctly redirect logs when running on mobile devices. Alternatively, if logging errors is useful when debugging and we do not care about bringing logs to users, I would recommend using the log package in the standard library, which defaults to printing on the standard error. We cannot print on the standard output because that is reserved for communication between ooniprobe and the Desktop app. Any message on the standard output that does not conform to the expected logging message would confuse the desktop app.
There was a problem hiding this comment.
understood, sorry about that. This one was probably a debugging slip; there were other places that were intended to print to console to mimick the ping utility, but this is a library and any output function should live in the cmd entrypoint. fixed.
| n, err := p.conn.Read(buf) | ||
| if err != nil { | ||
| var netErr *net.OpError | ||
| if errors.As(err, &netErr) && netErr.Timeout() { | ||
| if errors.As(err, &netErr) && netErr.Timeout() || err.Error() == "i/o timeout" { |
There was a problem hiding this comment.
Please, make sure we are matching the end of the error message by using strings.HasSuffix. It's quite common in Go to chain error messages and prefix additional context. For this reason, it would probably be better to try and use strings.HasSuffix. I would also recommend defining the specific error string in a constant rather than using the raw string directly inside the check.
| @@ -501,7 +508,7 @@ func (p *Pinger) recvICMP(recv chan<- *packet) error { | |||
| select { | |||
| case <-p.done: | |||
| return nil | |||
| case recv <- &packet{bytes: buf, nbytes: n}: | |||
| case recv <- &packet{bytes: buf[:], nbytes: n}: | |||
There was a problem hiding this comment.
The buf[:] idiom creates a new slice using the same underlying array. If the intent is to copy the array to make sure it is not otherwise modified, I think you want append([]byte{}, buf...).
Please, see this playground for more information about the difference between the two idioms.
Assuming the intent here is to duplicate the bytes, I would like to better understand why. Because we are not aiming for high high performance, I would say we should just allocate a new buffer for each message and accept that we transfer its ownership when we deliver it over a channel.
(You may have seen the buf[:] idiom being used when the backing buffer is a var buf [256]byte rather than a buf := make([]byte. 256). The reason why it's necessary to use buf[:] in such a case is that you have a function that accepts a []byte - i.e., a slice - and so you need to convert from an array to a slice, and you end up doing that using buf[:].)
| @@ -572,15 +585,47 @@ func (p *Pinger) processPacket(recv *packet) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (p *Pinger) parseEchoReplyFromICMP(data []byte, from *net.IP) *Packet { | |||
| replyPacket, err := icmp.ParseMessage(1, data[:]) | |||
| func (p *Pinger) parseEchoReplyFromICMP(data []byte, from *net.IP) *Packet { | ||
| replyPacket, err := icmp.ParseMessage(1, data[:]) | ||
| if err != nil { | ||
| log.Printf("error: %v\n", err) |
There was a problem hiding this comment.
Thank you for using log here. I'd like to encourage you to replace fmt.Printf with log.Printf throughout the code, otherwise we'll have troubles when we integrate this code with ooniprobe.
| replyPacket, err := icmp.ParseMessage(1, data[:]) | ||
| if err != nil { | ||
| log.Printf("error: %v\n", err) | ||
| return &Packet{} |
There was a problem hiding this comment.
I don't like this pattern much; I'd rather return *Packet, error or, if the error does not matter, a *Packet, bool mainly because I think we should leverage the fact that Go allows returning multiple values to avoid returning special-case canary values.
| if err := parser.DecodeLayers(data, &decoded); err != nil { | ||
| log.Printf("error: %v\n", err) | ||
| return &Packet{} | ||
| } |
There was a problem hiding this comment.
I admit I am a bit lost in what you are actually doing here. Would you mind adding some comments to the code explaining why you're parsing an ICMP packet and then apparently parsing it again a second time?
There was a problem hiding this comment.
It would also be useful to know, more in general, why we need a different parsing strategy with wireguard and we should perhaps consider whether we can adopt the same strategy also when using openvpn.
There was a problem hiding this comment.
Also, I am kinda lost in this thinking about why we're performing wireguard related changes in this library which does not seem to otherwise deal with wireguard. Is this because we're reusing openvpn code for wireguard?
| eventListener chan uint16 | ||
| eventListener chan uint8 | ||
|
|
||
| failed bool |
There was a problem hiding this comment.
what does this field do?
There was a problem hiding this comment.
this is an unrelated change, I will force-push (moved to #38)
some modifications to make ping work for the wireguard experiment branch