eX0-go/net.go: sendUDPPacketBytes: Investigate/confirm possible misuse of Read method.

[dmitshur]

@@ -94,6 +94,8 @@ func sendUDPPacketBytes(c *Connection, b []byte) error {
 func receiveUDPPacket(c *Connection) ([]byte, packet.UDPHeader, error) {
    var b [packet.MAX_UDP_SIZE]byte
    n, err := c.udp.Read(b[:])
+   // TODO: Use io.ReadFull or check b[n:] here? But it is UDP, so... Figure it out.
+   //       Maybe it's fine because it's Conn.Read, not io.Read?
    if err != nil {
        return nil, packet.UDPHeader{}, err
    }

Originally reported/found by @dominikh.

[dmitshur]

I'm not seeing a check that would report this in the latest version of staticcheck or anywhere. @dominikh, do you know/remember what happened to it? Is it still valid, or did you find some evidence otherwise?

[dominikh]

Same as in 2016: you don't need ReadFull because it's UDP, which is message based. You get a message or you don't, you don't get partial messages. You do need to use n because you don't know the size of the message ahead of time.

[dmitshur]

you don't need ReadFull because it's UDP, which is message based.

Yes, that's what assumed when I wrote the code, and it seems to work without issues. But it goes through an io.Reader-like interface (a subset of net.Conn), and I don't see any guarantees in docs of net.UDPConn.Read about a 1:1 mapping between packets and Read calls, I can't completely dismiss the possibility of a non-1:1 mapping (e.g., due to internal buffering, etc.).

I might raise an issue about it in the Go issue tracker then.

To be clear, I know how UDP packets work. I don't have full confidence on what happens when they go through io.Reader interface unless I see it written in docs.

[dominikh]

It's not "going through io.Reader", io.Reader isn't some entity sitting between your connection and you, messing with your data. And UDPConn implements net.Conn, not io.Reader. That aside, if it did split up UDP messages, it would be a 100% broken implementation and still not your concern.

But really, we've had this discussion a year ago, check your logs :-)

[dmitshur]

Is that why this never made it into a production-ready staticcheck check?

[dominikh]

Depends what you mean by "this". Not complaining about the early error check is probably because of UDP though. I don't recall all the details.