From 7ef97ef37e396d7f24df28ac5649ad19daf39592 Mon Sep 17 00:00:00 2001 From: Jordi Subira Nieto Date: Thu, 19 Dec 2024 14:04:31 +0100 Subject: [PATCH 1/2] ignore decoding/parsing errors in ReadFrom on SCIONPacketConn --- pkg/snet/packet_conn.go | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/pkg/snet/packet_conn.go b/pkg/snet/packet_conn.go index f8fd378929..89b53bc8b6 100644 --- a/pkg/snet/packet_conn.go +++ b/pkg/snet/packet_conn.go @@ -21,6 +21,7 @@ import ( "time" "github.com/scionproto/scion/pkg/addr" + "github.com/scionproto/scion/pkg/log" "github.com/scionproto/scion/pkg/metrics/v2" "github.com/scionproto/scion/pkg/private/common" "github.com/scionproto/scion/pkg/private/serrors" @@ -168,6 +169,14 @@ func (c *SCIONPacketConn) ReadFrom(pkt *Packet, ov *net.UDPAddr) error { if err != nil { return err } + if remoteAddr == nil { + // XXX(JordiSubira): The remote address of the underlay next host + // will not be nil unless there was an error while reading the + // SCION packet. If the err is nil, it means that it was a + // non-recoverable error (e.g., decoding the header) and we + // discard the packet and keep reading. + continue + } *ov = *remoteAddr if scmp, ok := pkt.Payload.(SCMPPayload); ok { if c.SCMPHandler == nil { @@ -206,7 +215,17 @@ func (c *SCIONPacketConn) readFrom(pkt *Packet) (*net.UDPAddr, error) { pkt.Bytes = pkt.Bytes[:n] if err := pkt.Decode(); err != nil { metrics.CounterInc(c.Metrics.ParseErrors) - return nil, serrors.Wrap("decoding packet", err) + // XXX(JordiSubira): We avoid bubbling up parsing errors to the + // caller application. This simulates that the network stack + // simply discards incorrect/malformated packets. Otherwise, + // some libraries/applications would misbehave (similar to what + // would happen if were to receive SCMP errors). + // + // If we believe that SCION-aware applications using the low-level + // SCIONPacketConn should receive the error, we can move this up to + // Conn. + log.Error("decoding packet", "error", err) + return nil, nil } udpRemoteAddr := remoteAddr.(*net.UDPAddr) @@ -218,7 +237,17 @@ func (c *SCIONPacketConn) readFrom(pkt *Packet) (*net.UDPAddr, error) { // *loopback:30041* `SCIONPacketConn.lastHop()` should yield the right next hop address. lastHop, err = c.lastHop(pkt) if err != nil { - return nil, serrors.Wrap("extracting last hop based on packet path", err) + // XXX(JordiSubira): We avoid bubbling up parsing errors to the + // caller application. This simulates that the network stack + // simply discards incorrect/malformated packets. Otherwise, + // some libraries/applications would misbehave (similar to what + // would happen if were to receive SCMP errors). + // + // If we believe that SCION-aware applications using the low-level + // SCIONPacketConn should receive the error, we can move this up to + // Conn. + log.Error("extracting last hop based on packet path", "error", err) + return nil, nil } } return lastHop, nil From 985881691e80fe7f5345a179192cde672a9f4161 Mon Sep 17 00:00:00 2001 From: Jordi Subira Nieto Date: Thu, 19 Dec 2024 15:53:40 +0100 Subject: [PATCH 2/2] pass --- pkg/snet/packet_conn.go | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/pkg/snet/packet_conn.go b/pkg/snet/packet_conn.go index 89b53bc8b6..493a125e2e 100644 --- a/pkg/snet/packet_conn.go +++ b/pkg/snet/packet_conn.go @@ -174,7 +174,7 @@ func (c *SCIONPacketConn) ReadFrom(pkt *Packet, ov *net.UDPAddr) error { // will not be nil unless there was an error while reading the // SCION packet. If the err is nil, it means that it was a // non-recoverable error (e.g., decoding the header) and we - // discard the packet and keep reading. + // discard the packet and keep continue } *ov = *remoteAddr @@ -216,15 +216,9 @@ func (c *SCIONPacketConn) readFrom(pkt *Packet) (*net.UDPAddr, error) { if err := pkt.Decode(); err != nil { metrics.CounterInc(c.Metrics.ParseErrors) // XXX(JordiSubira): We avoid bubbling up parsing errors to the - // caller application. This simulates that the network stack - // simply discards incorrect/malformated packets. Otherwise, - // some libraries/applications would misbehave (similar to what - // would happen if were to receive SCMP errors). - // - // If we believe that SCION-aware applications using the low-level - // SCIONPacketConn should receive the error, we can move this up to - // Conn. - log.Error("decoding packet", "error", err) + // caller application to avoid problems with applications + // that don't expect this type of errors. + log.Debug("decoding packet", "error", err) return nil, nil } @@ -238,15 +232,9 @@ func (c *SCIONPacketConn) readFrom(pkt *Packet) (*net.UDPAddr, error) { lastHop, err = c.lastHop(pkt) if err != nil { // XXX(JordiSubira): We avoid bubbling up parsing errors to the - // caller application. This simulates that the network stack - // simply discards incorrect/malformated packets. Otherwise, - // some libraries/applications would misbehave (similar to what - // would happen if were to receive SCMP errors). - // - // If we believe that SCION-aware applications using the low-level - // SCIONPacketConn should receive the error, we can move this up to - // Conn. - log.Error("extracting last hop based on packet path", "error", err) + // caller application to avoid problems with applications + // that don't expect this type of errors. + log.Debug("extracting last hop based on packet path", "error", err) return nil, nil } }