Skip to content

Commit

Permalink
remove error return value from ComposeVersionNegotiation (#3410)
Browse files Browse the repository at this point in the history
  • Loading branch information
dchaofei committed May 12, 2022
1 parent b9a77af commit 60bbe92
Show file tree
Hide file tree
Showing 8 changed files with 14 additions and 30 deletions.
6 changes: 2 additions & 4 deletions connection_test.go
Expand Up @@ -674,8 +674,7 @@ var _ = Describe("Connection", func() {
})

It("drops Version Negotiation packets", func() {
b, err := wire.ComposeVersionNegotiation(srcConnID, destConnID, conn.config.Versions)
Expect(err).ToNot(HaveOccurred())
b := wire.ComposeVersionNegotiation(srcConnID, destConnID, conn.config.Versions)
tracer.EXPECT().DroppedPacket(logging.PacketTypeVersionNegotiation, protocol.ByteCount(len(b)), logging.PacketDropUnexpectedPacket)
Expect(conn.handlePacketImpl(&receivedPacket{
data: b,
Expand Down Expand Up @@ -2592,8 +2591,7 @@ var _ = Describe("Client Connection", func() {

Context("handling Version Negotiation", func() {
getVNP := func(versions ...protocol.VersionNumber) *receivedPacket {
b, err := wire.ComposeVersionNegotiation(srcConnID, destConnID, versions)
Expect(err).ToNot(HaveOccurred())
b := wire.ComposeVersionNegotiation(srcConnID, destConnID, versions)
return &receivedPacket{
data: b,
buffer: getPacketBuffer(),
Expand Down
6 changes: 1 addition & 5 deletions fuzzing/header/cmd/corpus.go
Expand Up @@ -24,11 +24,7 @@ func getVNP(src, dest protocol.ConnectionID, numVersions int) []byte {
for i := 0; i < numVersions; i++ {
versions[i] = protocol.VersionNumber(rand.Uint32())
}
data, err := wire.ComposeVersionNegotiation(src, dest, versions)
if err != nil {
log.Fatal(err)
}
return data
return wire.ComposeVersionNegotiation(src, dest, versions)
}

func main() {
Expand Down
4 changes: 1 addition & 3 deletions fuzzing/header/fuzz.go
Expand Up @@ -91,8 +91,6 @@ func fuzzVNP(data []byte) int {
if len(versions) == 0 {
panic("no versions")
}
if _, err := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions); err != nil {
panic(err)
}
wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions)
return 1
}
2 changes: 1 addition & 1 deletion integrationtests/self/mitm_test.go
Expand Up @@ -297,7 +297,7 @@ var _ = Describe("MITM test", func() {
sendForgedVersionNegotationPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
// Create fake version negotiation packet with no supported versions
versions := []protocol.VersionNumber{}
packet, _ := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions)
packet := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions)

// Send the packet
_, err := conn.WriteTo(packet, remoteAddr)
Expand Down
4 changes: 2 additions & 2 deletions internal/wire/version_negotiation.go
Expand Up @@ -35,7 +35,7 @@ func ParseVersionNegotiationPacket(b *bytes.Reader) (*Header, []protocol.Version
}

// ComposeVersionNegotiation composes a Version Negotiation
func ComposeVersionNegotiation(destConnID, srcConnID protocol.ConnectionID, versions []protocol.VersionNumber) ([]byte, error) {
func ComposeVersionNegotiation(destConnID, srcConnID protocol.ConnectionID, versions []protocol.VersionNumber) []byte {
greasedVersions := protocol.GetGreasedVersions(versions)
expectedLen := 1 /* type byte */ + 4 /* version field */ + 1 /* dest connection ID length field */ + destConnID.Len() + 1 /* src connection ID length field */ + srcConnID.Len() + len(greasedVersions)*4
buf := bytes.NewBuffer(make([]byte, 0, expectedLen))
Expand All @@ -50,5 +50,5 @@ func ComposeVersionNegotiation(destConnID, srcConnID protocol.ConnectionID, vers
for _, v := range greasedVersions {
utils.BigEndian.WriteUint32(buf, uint32(v))
}
return buf.Bytes(), nil
return buf.Bytes()
}
13 changes: 5 additions & 8 deletions internal/wire/version_negotiation_test.go
Expand Up @@ -36,29 +36,26 @@ var _ = Describe("Version Negotiation Packets", func() {
It("errors if it contains versions of the wrong length", func() {
connID := protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}
versions := []protocol.VersionNumber{0x22334455, 0x33445566}
data, err := ComposeVersionNegotiation(connID, connID, versions)
Expect(err).ToNot(HaveOccurred())
_, _, err = ParseVersionNegotiationPacket(bytes.NewReader(data[:len(data)-2]))
data := ComposeVersionNegotiation(connID, connID, versions)
_, _, err := ParseVersionNegotiationPacket(bytes.NewReader(data[:len(data)-2]))
Expect(err).To(MatchError("Version Negotiation packet has a version list with an invalid length"))
})

It("errors if the version list is empty", func() {
connID := protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}
versions := []protocol.VersionNumber{0x22334455}
data, err := ComposeVersionNegotiation(connID, connID, versions)
Expect(err).ToNot(HaveOccurred())
data := ComposeVersionNegotiation(connID, connID, versions)
// remove 8 bytes (two versions), since ComposeVersionNegotiation also added a reserved version number
data = data[:len(data)-8]
_, _, err = ParseVersionNegotiationPacket(bytes.NewReader(data))
_, _, err := ParseVersionNegotiationPacket(bytes.NewReader(data))
Expect(err).To(MatchError("Version Negotiation packet has empty version list"))
})

It("adds a reserved version", func() {
srcConnID := protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef, 0xca, 0xfe, 0x13, 0x37}
destConnID := protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}
versions := []protocol.VersionNumber{1001, 1003}
data, err := ComposeVersionNegotiation(destConnID, srcConnID, versions)
Expect(err).ToNot(HaveOccurred())
data := ComposeVersionNegotiation(destConnID, srcConnID, versions)
Expect(data[0] & 0x80).ToNot(BeZero())
hdr, supportedVersions, err := ParseVersionNegotiationPacket(bytes.NewReader(data))
Expect(err).ToNot(HaveOccurred())
Expand Down
6 changes: 1 addition & 5 deletions server.go
Expand Up @@ -651,11 +651,7 @@ func (s *baseServer) sendError(remoteAddr net.Addr, hdr *wire.Header, sealer han

func (s *baseServer) sendVersionNegotiationPacket(p *receivedPacket, hdr *wire.Header) {
s.logger.Debugf("Client offered version %s, sending Version Negotiation", hdr.Version)
data, err := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, s.config.Versions)
if err != nil {
s.logger.Debugf("Error composing Version Negotiation: %s", err)
return
}
data := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, s.config.Versions)
if s.config.Tracer != nil {
s.config.Tracer.SentPacket(
p.remoteAddr,
Expand Down
3 changes: 1 addition & 2 deletions server_test.go
Expand Up @@ -426,12 +426,11 @@ var _ = Describe("Server", func() {
})

It("ignores Version Negotiation packets", func() {
data, err := wire.ComposeVersionNegotiation(
data := wire.ComposeVersionNegotiation(
protocol.ConnectionID{1, 2, 3, 4},
protocol.ConnectionID{4, 3, 2, 1},
[]protocol.VersionNumber{1, 2, 3},
)
Expect(err).ToNot(HaveOccurred())
raddr := &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 1337}
done := make(chan struct{})
tracer.EXPECT().DroppedPacket(raddr, logging.PacketTypeVersionNegotiation, protocol.ByteCount(len(data)), logging.PacketDropUnexpectedPacket).Do(func(net.Addr, logging.PacketType, protocol.ByteCount, logging.PacketDropReason) {
Expand Down

0 comments on commit 60bbe92

Please sign in to comment.