search

go-ping / ping

ICMP Ping library for Go
MIT License
1.33k stars 345 forks source link

Wrap sequence numbers #142

Closed SuperQ closed 1 year ago

SuperQ commented 3 years ago

ICMP sequence numbers are only 16 bits, so we need to wrap the counter when we hit 65535.

See: https://github.com/SuperQ/smokeping_prober/issues/51

CHTJonas commented 3 years ago

Go's uint16 should wrap as expected (https://play.golang.org/p/mRbqYpALimF) so I think we just need to change the datatype from int to that:

https://github.com/go-ping/ping/blob/ab39f29b51f8737862e1b692f31fbcc4ba87d290/ping.go#L176

Does this raise concerns about the inflight tracking in the awaitingSequences map? My own view is that if someone sends 65536 packets in the space of a RTT timeout then that's their own fault.

SuperQ commented 3 years ago

Yes, it's going to possibly need some extra checks around the awaitingSequences. :-(

Another way to handle this is to reset the sequence number and generate a new tracking ID.

awaitingSequences can be a map[trackingId][int]struct{}{}. Then we'll have to make the ID a []trackingId.

Another good reason to use UUID for the tracking id.

SuperQ commented 3 years ago

Maybe @jraby wants to work on this one. :smile: