Server / client should implement some sanity checks to prevent unbounded memory growth

[gideonw]

Potential client disconnect memory leak where we need to clean up resources explicitly

[gideonw]

I actually believe this is due to length being parsed incorrectly and then allocating a massive buffer. We might need to make some sane checks here.

Possibly #99

	length := binary.BigEndian.Uint32(lengthPrefix)
	b := make([]byte, length)
	n, err := io.ReadFull(r, b)

[dburkart]

Yeah that would make sense if there was mismatch between server and client (one was little and the other big).

I guess there's one clever thing we could do which is to reject any client that looks to be little endian during version negotiation. The lengths should always be the same (for v1.0.0 anyway), so we can check for a specific length. This would at least make it easier to not get in trouble with mismatched dev versions.

[gideonw]

I'm also thinking about potential bad actors here too. You're right though, this shouldn't be an issue in the future.

[gideonw]

@dburkart This also comes about if someone opens a TCP connection and sends data that isn't our proto. Do you think it makes sense to add a magic first byte to the version message or the message in general to immediately disregard malformed requests.

[dburkart]

Moving to 0.1.4 for investigation.

[dburkart]

Which resources are not being cleaned up? I checked and it looks like we clean up the net.TCPConn with a defer, but we don't explicitly nil out any fields on the conn struct. Not sure if that's necessary tho

[gideonw]

I think that it is more likely the symptoms are from what I mention over chat. The first byte being used for the length of the buffer causing a massive consumption of memory of it isn't the proper format.

[dburkart]

Ahh right. Let me take a look at that.

[dburkart]

Fixed the main issue here by backporting the big-endian change to release/0.1.x. Will use this issue to track some defensive programming around message sizes and the like.