fix wrong SizeOfTCPInfo #643
Conversation
| @@ -352,7 +352,7 @@ type TCPInfo struct { | |||
| } | |||
|
|
|||
| // SizeOfTCPInfo is the binary size of a TCPInfo struct. | |||
| const SizeOfTCPInfo = 104 | |||
| const SizeOfTCPInfo = 192 | |||
There was a problem hiding this comment.
Is there a reference in the linux source code or something like that that determines this size?
There was a problem hiding this comment.
On my system:
sizeof(struct tcp_info) = 104
I think the problem is our TCPInfo struct, not this constant.
There was a problem hiding this comment.
IMO, even we got 104 bytes from system, this still breaks the Unmarshal as we have a larger size of TCPInfo struct.
According to kernel (https://github.com/torvalds/linux/blob/master/net/ipv4/tcp.c#L3431), and ss (https://github.com/sivasankariit/iproute2/blob/master/misc/ss.c#L1378), it seems that we shall have a similar workaround.
There was a problem hiding this comment.
I believe that 104 bytes is the new larger size and that struct tcp_info was previously smaller. It would appear that the problem is that our TCPInfo struct has a bunch of extra fields.
There was a problem hiding this comment.
Nevermind. It looks like the struct was correct at a time and that the newest one is even larger still. What really confuses me is that version in my glibc seems to have fields missing from the middle as well. It appears that they added an extra field based on the assumption that it wouldn't change the size due to the space previously having been there as padding.
The code change looks good to me.
Could you please also:
Add a test which verifies that this constant contains the correct value.
Add a comment that this struct grew before this snapshot of it was take and has grown since.
Add a comment that this snapshot of the struct is from Linux 4.18.
There was a problem hiding this comment.
I effectively merged this in 691c2f8
There was a problem hiding this comment.
I effectively merged this in 691c2f8
Would you mind adding the comments as suggested by Ian (see above)?
Below command under hostinet network will lead to panic: $ cat /proc/net/tcp It's caused by the wrong SizeOfTCPInfo. #0 runtime.panicindex() google#1 encoding/binary.littleEndian.Uint64 google#2 encoding/binary.(*littleEndian).Uint64 google#3 gvisor.dev/gvisor/pkg/binary.unmarshal google#4 gvisor.dev/gvisor/pkg/binary.unmarshal google#5 gvisor.dev/gvisor/pkg/binary.Unmarshal google#6 gvisor.dev/gvisor/pkg/sentry/socket/hostinet.(*socketOperations).State google#7 gvisor.dev/gvisor/pkg/sentry/fs/proc.(*netTCP).ReadSeqFileData Correct SizeOfTCPInfo from 104 to 192 to fix it. Fixes google#640 Signed-off-by: Jianfeng Tan <henry.tjf@antfin.com>
Below command under hostinet network will lead to panic:
$ cat /proc/net/tcp
It's caused by the wrong SizeOfTCPInfo.
#0 runtime.panicindex()
#1 encoding/binary.littleEndian.Uint64
#2 encoding/binary.(*littleEndian).Uint64
#3 gvisor.dev/gvisor/pkg/binary.unmarshal
#4 gvisor.dev/gvisor/pkg/binary.unmarshal
#5 gvisor.dev/gvisor/pkg/binary.Unmarshal
#6 gvisor.dev/gvisor/pkg/sentry/socket/hostinet.(*socketOperations).State
#7 gvisor.dev/gvisor/pkg/sentry/fs/proc.(*netTCP).ReadSeqFileData
Correct SizeOfTCPInfo from 104 to 192 to fix it.
Signed-off-by: Jianfeng Tan henry.tjf@antfin.com