ICMP tests and various fixes (#68)

* various icmp fixes

* improve existing tests

internet/stack-ip.go

@@ -105,9 +105,6 @@ func (sb *StackIP) Demux(carrierData []byte, offset int) error {
 	off := ifrm.HeaderLength()
 	totalLen := ifrm.TotalLength()
 	proto := ifrm.Protocol()
-	if proto == lneto.IPProtoICMP {
-		return sb.recvicmp(ifrm.Payload())
-	}
 	node := sb.handlers.nodeByProto(uint16(proto))
 	// nodeIdx := getNodeByProto(sb.handlers, uint16(proto))
 	if node == nil {

ipv4/icmpv4/client.go

@@ -188,6 +188,7 @@ func (client *Client) Encapsulate(carrierData []byte, ipOffset, frameOffset int)
 		}
 		copy(data[written:written+size%len(pattern)], pattern)
 		n = sizeHeader + size
+		out.key |= keyHashSentBit
 		raddr = out.raddr
 	} else {
 		return 0, nil
@@ -198,8 +199,8 @@ func (client *Client) Encapsulate(carrierData []byte, ipOffset, frameOffset int)
 	var crc lneto.CRC791
 	sum := crc.PayloadSum16(carrierData[frameOffset : frameOffset+n])
 	ifrm.SetCRC(sum)
-	if frameOffset >= 20 {
+	if ipOffset >= 0 {
-		err = internal.SetIPAddrs(carrierData, 0, nil, raddr[:])
+		err = internal.SetIPAddrs(carrierData[ipOffset:], 0, nil, raddr[:])
 	}
 	return n, err
 }

ipv4/icmpv4/client_test.go

@@ -18,6 +18,7 @@ func TestClients(t *testing.T) {
 		ResponseQueueBuffer: make([]byte, sizebuffer),
 		ResponseQueueLimit:  queuesize,
 		HashSeed:            testHashSeed,
+		ID:                  1,
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -26,6 +27,7 @@ func TestClients(t *testing.T) {
 		ResponseQueueBuffer: make([]byte, sizebuffer),
 		ResponseQueueLimit:  queuesize,
 		HashSeed:            testHashSeed,
+		ID:                  2,
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -38,13 +40,30 @@ func TestClients(t *testing.T) {
 	if !completed || !ok {
 		t.Fatal("ping did not complete or not exist")
 	}
+	n, err := sender.Encapsulate(buf[:], -1, 0)
+	if err != nil {
+		t.Error(err)
+	} else if n > 0 {
+		t.Error("sender: expected no more data to be sent")
+	}
+	n, err = responder.Encapsulate(buf[:], -1, 0)
+	if err != nil {
+		t.Error(err)
+	} else if n > 0 {
+		t.Error("responder: expected no more data to be sent")
+	}
 }
 
 func testSingleExchange(t *testing.T, sender, responder *Client, buf []byte, pattern []byte, size uint16) (senderKey uint32) {
-	var n int
-	senderKey, n = testSendEcho(t, sender, buf, pattern, size)
 	const frameOff = 0
-	const ipOff = 0
+	const ipOff = -1
+	n, err := responder.Encapsulate(buf, ipOff, frameOff)
+	if err != nil {
+		t.Fatal(err)
+	} else if n > 0 {
+		t.Fatal("expected no data pending to be sent from responder")
+	}
+	senderKey, n = testSendEcho(t, sender, buf, pattern, size)
 	completed, ok := sender.PingPeek(senderKey)
 	if !ok {
 		t.Error("ping key not exist")
@@ -58,7 +77,7 @@ func testSingleExchange(t *testing.T, sender, responder *Client, buf []byte, pat
 	if err1 != nil {
 		t.Error("responder demux during single", err1)
 	}
-	n, err := responder.Encapsulate(buf, ipOff, frameOff)
+	n, err = responder.Encapsulate(buf, ipOff, frameOff)
 	if err != nil {
 		t.Error("responder encaps during single", err)
 		return
@@ -96,17 +115,31 @@ func testSingleExchange(t *testing.T, sender, responder *Client, buf []byte, pat
 	if completed2, ok2 := sender.PingPeek(senderKey); completed != completed2 || ok != ok2 {
 		t.Error("change in status after peek")
 	}
+	n, err = sender.Encapsulate(buf, ipOff, frameOff)
+	if err != nil {
+		t.Error("error after done")
+	}
+	if n > 0 {
+		t.Error("expected no data to be sent after ping completion", n)
+	}
 	return senderKey
 }
 
 func testSendEcho(t *testing.T, sender *Client, buf []byte, pattern []byte, size uint16) (key uint32, n int) {
 	t.Helper()
-	key, err := sender.PingStart([4]byte{1}, pattern, size)
+	const frameOff = 0
+	const ipOff = -1
+	n, err := sender.Encapsulate(buf, ipOff, frameOff)
+	if err != nil {
+		t.Fatal(err)
+	} else if n > 0 {
+		t.Fatal("expected no data pending to send on testSendEcho start")
+	}
+	key, err = sender.PingStart([4]byte{1}, pattern, size)
 	if err != nil {
 		t.Fatal(err)
 	}
-	const frameOff = 0
+
-	const ipOff = 0
 	n, err = sender.Encapsulate(buf[:], ipOff, frameOff)
 	if err != nil {
 		t.Errorf("sender encapsulate: %v", err)

ntp/ntp.go

@@ -5,7 +5,6 @@ import (
 	"encoding/binary"
 	"math"
 	"math/bits"
-	"sync"
 	"time"
 
 	"github.com/soypat/lneto"
@@ -265,24 +264,21 @@ func (d Date) Time() (time.Time, error) {
 	return baseTime.Add(off), nil
 }
 
-var (
-	ntpOnceSystemClock sync.Once
-	sysPrec            int8
-)
-
 // CalculateSystemPrecision calculates the NTP system precision for a time source.
-// If the time source is nil the default static call to [time.Now] is used.
+// If the time source is nil the default static call to [time.Now]->[time.Time.UnixNano] is used.
-func CalculateSystemPrecision(now func() time.Time, iters []time.Time) int8 {
+func CalculateSystemPrecision(nowNano func() int64, iters []int64) int8 {
 	maxIter := len(iters)
-	if now == nil {
+	if nowNano == nil {
 		for i := 0; i < maxIter; i++ {
-			iters[i] = time.Now()
+			iters[i] = time.Now().UnixNano()
 		}
 	} else {
 		for i := 0; i < maxIter; i++ {
-			iters[i] = now()
+			iters[i] = nowNano()
 		}
 	}
-	avg := iters[maxIter-1].Sub(iters[0]) / time.Duration(maxIter)
+	const seconds = 1_000_000_000 // nanoseconds
-	return int8(math.Log2(avg.Seconds()))
+	avg := (iters[maxIter-1] - iters[0]) / int64(maxIter)
+	avgSeconds := float64(avg) / seconds
+	return int8(math.Log2(avgSeconds))
 }

x/xnet/stack-async.go

@@ -132,7 +132,7 @@ func (s *StackAsync) MTU() int {
 }
 
 func (s *StackAsync) Reset(cfg StackConfig) error {
-	if cfg.RandSeed == 0 {
+	if cfg.RandSeed == 0 || cfg.Hostname == "" {
 		return lneto.ErrInvalidConfig
 	}
 	mac := cfg.HardwareAddress
@@ -203,14 +203,15 @@ func (s *StackAsync) Reset(cfg StackConfig) error {
 		err = s.icmp.Configure(icmpv4.ClientConfig{
 			ResponseQueueBuffer: make([]byte, cfg.ICMPQueueLimit*64),
 			ResponseQueueLimit:  cfg.ICMPQueueLimit,
-			HashSeed:            s.Prand32(),
+			HashSeed:            s.prand32(),
+			ID:                  uint16(cfg.Hostname[len(cfg.Hostname)-1]) - '0', // Treat last character of hostname as number.
 		})
 		if err != nil {
 			return err
 		}
 	}
-	var timebuf [32]time.Time
+	var timebuf [4]int64
-	s.sysprec = ntp.CalculateSystemPrecision(time.Now, timebuf[:])
+	s.sysprec = ntp.CalculateSystemPrecision(nil, timebuf[:])
 	if s.clientID == "" {
 		s.clientID = "lneto-" + s.hostname
 	}
@@ -335,11 +336,9 @@ func (s *StackAsync) Gateway6() [6]byte {
 func (s *StackAsync) EnableICMP(enabled bool) (err error) {
 	if enabled {
 		if s.ip.IsRegistered(lneto.IPProtoICMP) {
-			err = lneto.ErrAlreadyRegistered
+			return nil
-		} else {
-			err = s.ip.Register(&s.icmp)
 		}
-
+		err = s.ip.Register(&s.icmp)
 	} else {
 		s.icmp.Abort()
 	}

x/xnet/xnet_icmp_test.go

@@ -0,0 +1,123 @@
+package xnet
+
+import (
+	"net/netip"
+	"testing"
+)
+
+func TestStackAsync_ICMPEcho(t *testing.T) {
+	tests := []struct {
+		name    string
+		pattern []byte
+		size    uint16
+	}{
+		{"1 EchoRequestReply", []byte("icmp-test"), 56},
+		{"2 ReplyToManualRequest", []byte("manual-echo"), 32},
+	}
+	sender, receiver := newICMPStacks(t, 42, 1500) // seed 42 is arbitrary, stacks get unique IPs
+	if err := sender.EnableICMP(true); err != nil {
+		t.Fatal(err)
+	}
+	if err := receiver.EnableICMP(true); err != nil {
+		t.Fatal(err)
+	}
+	buf := make([]byte, 2048)
+	n, err := sender.EgressEthernet(buf)
+	if n > 0 || err != nil {
+		t.Fatal("sender unexpected data sent or error", n, err)
+	}
+	n, err = receiver.EgressEthernet(buf)
+	if n > 0 || err != nil {
+		t.Fatal("receiver unexpected data sent or error", n, err)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			sender.SetGateway6(receiver.HardwareAddress())
+			receiver.SetGateway6(sender.HardwareAddress())
+
+			key, err := sender.icmp.PingStart(receiver.Addr().As4(), tt.pattern, tt.size)
+			if err != nil {
+				t.Fatal(err)
+			}
+			echoSent := exchangeEthernetOnce(t, sender, receiver, buf)
+			if !echoSent {
+				t.Error("ECHO not sent")
+			}
+			echoReplySent := exchangeEthernetOnce(t, receiver, sender, buf)
+			if !echoReplySent {
+				t.Error("ECHOREPLY not sent")
+			}
+			n, err = sender.EgressEthernet(buf)
+			if n > 0 || err != nil {
+				t.Error("sender excess data sent or error", n, err)
+			}
+			n, err = receiver.EgressEthernet(buf)
+			if n > 0 || err != nil {
+				t.Error("receiver excess data sent or error", n, err)
+			}
+
+			completed, ok := sender.icmp.PingPop(key)
+			if !ok {
+				t.Fatal("ping key not found")
+			}
+			if !completed {
+				t.Fatal("expected ping to complete")
+			}
+		})
+	}
+}
+
+// exchangeEthernetOnce sends one Ethernet frame from src to dst if available.
+func exchangeEthernetOnce(t *testing.T, src, dst *StackAsync, buf []byte) bool {
+	t.Helper()
+	n, err := src.EgressEthernet(buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n == 0 {
+		return false
+	}
+	if err := dst.IngressEthernet(buf[:n]); err != nil {
+		t.Fatal(err)
+	}
+	return true
+}
+
+// newICMPStacks creates two test stacks with distinct static addresses and hardware addresses.
+func newICMPStacks(t testing.TB, randSeed int64, mtu int) (*StackAsync, *StackAsync) {
+	const icmpQueue = 4
+	s1, s2 := new(StackAsync), new(StackAsync)
+
+	// Use the seed to generate two adjacent IPs (10.0.0.x) and MACs.
+	base := byte(randSeed & 0x7F) // keep in safe range 0..127
+	addr1 := netip.AddrFrom4([4]byte{10, 0, 0, base})
+	addr2 := netip.AddrFrom4([4]byte{10, 0, 0, base + 1})
+	mac1 := [6]byte{0xbe, 0xef, 0, 0, 0, base}
+	mac2 := [6]byte{0xbe, 0xef, 0, 0, 0, base + 1}
+
+	if err := s1.Reset(StackConfig{
+		Hostname:        "icmp-stack-1",
+		RandSeed:        randSeed,
+		StaticAddress:   addr1,
+		HardwareAddress: mac1,
+		MTU:             uint16(mtu),
+		ICMPQueueLimit:  icmpQueue,
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := s2.Reset(StackConfig{
+		Hostname:        "icmp-stack-2",
+		RandSeed:        ^randSeed,
+		StaticAddress:   addr2,
+		HardwareAddress: mac2,
+		MTU:             uint16(mtu),
+		ICMPQueueLimit:  icmpQueue,
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	return s1, s2
+}

x/xnet/xnet_test.go

@@ -11,9 +11,11 @@ import (
 
 	"github.com/soypat/lneto/arp"
 	"github.com/soypat/lneto/ethernet"
+	"github.com/soypat/lneto/internal"
 	"github.com/soypat/lneto/internal/ltesto"
 	"github.com/soypat/lneto/internet/pcap"
 	"github.com/soypat/lneto/ipv4"
+	"github.com/soypat/lneto/ipv4/icmpv4"
 	"github.com/soypat/lneto/tcp"
 )
 
@@ -913,11 +915,12 @@ func TestTCPConn_BufferNotClearedOnPassiveClose(t *testing.T) {
 
 func TestStackAsync_ICMPEchoChecksum(t *testing.T) {
 	const MTU = 1500
+	const MaxFrameLength = MTU + 14 + 4 // Ethernet header+FCS.
 	stackAddr := netip.AddrFrom4([4]byte{192, 168, 1, 99})
 	stackMAC := [6]byte{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}
 	routerAddr := [4]byte{192, 168, 1, 1}
 	routerMAC := [6]byte{0x00, 0x11, 0x22, 0x33, 0x44, 0x55}
-
+	var rawbuf [MaxFrameLength]byte
 	stack := new(StackAsync)
 	err := stack.Reset(StackConfig{
 		Hostname:        "ICMPTest",
@@ -925,11 +928,15 @@ func TestStackAsync_ICMPEchoChecksum(t *testing.T) {
 		StaticAddress:   stackAddr,
 		HardwareAddress: stackMAC,
 		MTU:             MTU,
+		ICMPQueueLimit:  2,
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
-
+	err = stack.EnableICMP(true)
+	if err != nil {
+		t.Error("enabling ICMP:", err)
+	}
 	gen := ltesto.PacketGen{
 		SrcMAC:  routerMAC,
 		DstMAC:  stackMAC,
@@ -937,22 +944,35 @@ func TestStackAsync_ICMPEchoChecksum(t *testing.T) {
 		DstIPv4: stackAddr.As4(),
 	}
 	icmpPayload := []byte("abcdefghijklmnopqrstuvwxyz012345") // 32 bytes, typical ping payload.
-
+	const (
+		id  = 0x1234
+		seq = 1
+	)
 	// Test 1: Valid ICMP echo request should be accepted.
-	pkt := gen.AppendIPv4ICMPEcho(nil, ltesto.ICMPEchoConfig{
+	pkt := gen.AppendIPv4ICMPEcho(rawbuf[:0], ltesto.ICMPEchoConfig{
-		Identifier:     0x1234,
+		Identifier:     id,
-		SequenceNumber: 1,
+		SequenceNumber: seq,
 		Payload:        icmpPayload,
 	})
 	err = stack.IngressEthernet(pkt)
 	if err != nil {
 		t.Fatalf("valid ICMP echo rejected: %v", err)
 	}
-
+	n, err := stack.EgressEthernet(rawbuf[:])
+	if err != nil || n == 0 {
+		t.Error("expected ICMP response:", n, err)
+	}
+	ifrm, err := icmpv4.NewFrame(rawbuf[14+20 : n])
+	efrm := icmpv4.FrameEcho{Frame: ifrm}
+	if err != nil {
+		t.Fatal(err)
+	} else if efrm.Identifier() != id || efrm.SequenceNumber() != seq || !internal.BytesEqual(icmpPayload, efrm.Data()) {
+		t.Errorf("id want %d, got %d; seq want %d, got %d, payload want %q, got %q", id, efrm.Identifier(), seq, efrm.SequenceNumber(), icmpPayload, efrm.Data())
+	}
 	// Test 2: Valid ICMP with trailing FCS bytes (simulates real PIO hardware capture).
 	// This is a regression test for the bug where recvicmp checksummed ifrm.RawData()
 	// instead of ifrm.Payload(), causing the 4 trailing FCS bytes to corrupt the checksum.
-	pkt = gen.AppendIPv4ICMPEcho(nil, ltesto.ICMPEchoConfig{
+	pkt = gen.AppendIPv4ICMPEcho(rawbuf[:0], ltesto.ICMPEchoConfig{
 		Identifier:     0x1234,
 		SequenceNumber: 2,
 		Payload:        icmpPayload,
@@ -964,7 +984,7 @@ func TestStackAsync_ICMPEchoChecksum(t *testing.T) {
 	}
 
 	// Test 3: Corrupted ICMP checksum should be rejected.
-	pkt = gen.AppendIPv4ICMPEcho(nil, ltesto.ICMPEchoConfig{
+	pkt = gen.AppendIPv4ICMPEcho(rawbuf[:0], ltesto.ICMPEchoConfig{
 		Identifier:     0x1234,
 		SequenceNumber: 3,
 		Payload:        icmpPayload,
@@ -976,7 +996,7 @@ func TestStackAsync_ICMPEchoChecksum(t *testing.T) {
 	}
 
 	// Test 4: Corrupted ICMP with trailing FCS should also be rejected.
-	pkt = gen.AppendIPv4ICMPEcho(nil, ltesto.ICMPEchoConfig{
+	pkt = gen.AppendIPv4ICMPEcho(rawbuf[:0], ltesto.ICMPEchoConfig{
 		Identifier:     0x1234,
 		SequenceNumber: 4,
 		Payload:        icmpPayload,