Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to send packets size with 60000? #34

Closed
hel2o opened this issue Oct 12, 2018 · 5 comments
Closed

How to send packets size with 60000? #34

hel2o opened this issue Oct 12, 2018 · 5 comments

Comments

@hel2o
Copy link

hel2o commented Oct 12, 2018

when i set pinger.Size = 60000 ,but packets is not send
default

default

@sparrc
Copy link
Member

sparrc commented Nov 21, 2018

I believe this may have been fixed by #45, can you update to the latest commit of go-ping and try again?

@SuperQ
Copy link
Contributor

SuperQ commented Nov 21, 2018

I think there are still issues with the size option.

When I set p.Size = 256, tcpdump says the packet length is 394.

When I set p.Size = 321, tcpdump says the packet length is 478.

When I set p.Size = 322, tcpdump says the packet length is 482, and panics my prober.

panic: runtime error: slice bounds out of range

goroutine 39 [running]:
github.com/superq/smokeping_prober/ping.(*Pinger).processPacket(0xc000159110, 0xc0001609c0, 0x4, 0x3)
	/home/ben/go/src/github.com/superq/smokeping_prober/ping/ping.go:435 +0x6be
github.com/superq/smokeping_prober/ping.(*Pinger).run(0xc000159110)
	/home/ben/go/src/github.com/superq/smokeping_prober/ping/ping.go:321 +0x4e1
github.com/superq/smokeping_prober/ping.(*Pinger).Run(0xc000159110)
	/home/ben/go/src/github.com/superq/smokeping_prober/ping/ping.go:270 +0x2b
created by main.main
	/home/ben/go/src/github.com/superq/smokeping_prober/main.go:95 +0xf09

@sparrc
Copy link
Member

sparrc commented Nov 21, 2018

I'm going to hazard a guess that this has something to do with the code still referencing timeSliceLength, which is used as a constant to initialize the size of the packet sent. This constant likely needs to be removed and the logic around it changed.

https://github.com/sparrc/go-ping/blob/ef3ab45e41b017889941ea5bb796dd502d2b5a1b/ping.go#L514

@SuperQ
Copy link
Contributor

SuperQ commented Nov 21, 2018

Yea, that bit didn't look right to me at all, but I didn't have time to poke around.

@CHTJonas
Copy link
Member

Using the latest commit on master and with sudo tcpdump icmp running in another terminal tab:

  • pinger.Size = 256 logs an ICMP echo request packet with length 264.
  • pinger.Size = 321 logs an ICMP echo request packet with length 329.
  • pinger.Size = 322 logs an ICMP echo request packet with length 330.
  • pinger.Size = 1472 logs an ICMP echo request packet with length 1480.

ICMP packets have an 8-byte header so this all seams correct to me.

If you set a (IMHO silly) packet size pinger.Size = 60000 (or indeed anything above 1472 in most instances) then the ICMP datagram gets fragmented across multiple IP packets at the network MTU. I don't really think we can expect things to behave reliably is such circumstances so if the user sets Size > MTU then they get to keep both pieces.

@hel2o hel2o closed this as completed Oct 19, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants