dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.07k stars 4.69k forks source link

macOS 11: System.Net.NetworkInformation.Tests.IPGlobalPropertiesTest.IPGlobalProperties_AccessAllMethods_NoErrors failure #40938

Closed directhex closed 4 years ago

directhex commented 4 years ago

Mono and CoreCLR both fail near-identically on this test on Helix.

CoreCLR https://helix.dot.net/api/2019-06-17/jobs/4f122c80-6e43-48c3-9793-d5c72dc59bbd/workitems/System.Net.NetworkInformation.Functional.Tests/console

     System.Net.NetworkInformation.Tests.IPGlobalPropertiesTest.IPGlobalProperties_AccessAllMethods_NoErrors [FAIL]
      System.Net.NetworkInformation.NetworkInformationException : An error was encountered while querying information from the operating system.
      Stack Trace:
        /_/src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdIcmpV6Statistics.cs(42,0): at System.Net.NetworkInformation.BsdIcmpV6Statistics..ctor()
        /_/src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdIPGlobalProperties.cs(123,0): at System.Net.NetworkInformation.BsdIPGlobalProperties.GetIcmpV6Statistics()
        /_/src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/IPGlobalPropertiesTest.cs(46,0): at System.Net.NetworkInformation.Tests.IPGlobalPropertiesTest.IPGlobalProperties_AccessAllMethods_NoErrors()

Mono https://helix.dot.net/api/2019-06-17/jobs/6648e68b-974b-4787-9345-e0223c696d09/workitems/System.Net.NetworkInformation.Functional.Tests/console

    System.Net.NetworkInformation.Tests.IPGlobalPropertiesTest.IPGlobalProperties_AccessAllMethods_NoErrors [FAIL]
      System.Net.NetworkInformation.NetworkInformationException : An error was encountered while querying information from the operating system.
      Stack Trace:
        /_/src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdIcmpV6Statistics.cs(73,0): at System.Net.NetworkInformation.BsdIcmpV6Statistics..ctor()
        /_/src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdIPGlobalProperties.cs(123,0): at System.Net.NetworkInformation.BsdIPGlobalProperties.GetIcmpV6Statistics()
        /_/src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/IPGlobalPropertiesTest.cs(46,0): at System.Net.NetworkInformation.Tests.IPGlobalPropertiesTest.IPGlobalProperties_AccessAllMethods_NoErrors()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(384,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

Something about how this data is parsed from the OS needs fixing up for macOS 11.

ghost commented 4 years ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

danmoseley commented 4 years ago

Do we need any change here in the Sept servicing? That is about to be code complete (well it was last Friday). I'm guessing that the Sept patch may be the latest available whan macOS 11 releases.

wfurt commented 4 years ago

I could not reproduce it locally

  === TEST EXECUTION SUMMARY ===
     System.Net.NetworkInformation.Functional.Tests  Total: 240, Errors: 0, Failed: 0, Skipped: 0, Time: 0.735s
  ~/Downloads/runtime/src/libraries/System.Net.NetworkInformation/tests/FunctionalTests

furt@macos-11 FunctionalTests % sw_vers
ProductName:    macOS
ProductVersion: 11.0
BuildVersion:   20A5343i

can we verify that IPv6 is configured on the CI system?

directhex commented 4 years ago

@MattGal do we have an answer to that ^^ ?

And is IPv6 configured on any of the pre-Big-Sur Mac queues? i.e. has the behavior changed regarding unconfigured IPv6, or is that a config difference?

MattGal commented 4 years ago

@directhex not a mac expert, but a quick ifconfig looks like IPv6 is enabled on all the active adapters: image

I have KVM access to the macs so if you want me to check a specific thing just let me know. Edit: with exception of the two you noted :(

danmoseley commented 4 years ago

Would it be helpful to have a test that does nothing but verifies that IPv6 is enabled?

wfurt commented 4 years ago

There was old issue to cleanup tests to not fail if IPv6 is not available. (sockets & HTTP) But we did not get to it as priority ;(

If @MattGal has direct access, perhaps we could try to run the tests directly and see if that would fail. Without repro, it would be hard to diagnose. Lack of IPv6 was guess since it failed on IPv6 path.

directhex commented 4 years ago

It looks like Apple changed the size of the icmp6stat struct, so code which just gets net.inet6.icmp6.stats and puts it in a icmp6stat will fail if compiled on pre-Big-Sur and executed on Big Sur. At least as far back as High Sierra, the return value from sysctl was 4336 bytes long, and now it's 4344.

akoeplinger commented 4 years ago

The usages of systemctlbyname should probably first call it with NULL to get the size of the buffer so we know the correct size. Some functions in pal_networkstatistics.c actually do this already.

wfurt commented 4 years ago

That make sense @directhex. I did build on Big Sur and that is probably reason whey it worked. I will check what changed so we map values correctly. Hopefully there are only additions.

rootwyrm commented 4 years ago

Oof. So it looks like Apple rebased partially somewhere in there. Normally it's a pretty clean xref with FreeBSD. However, <=12.1 is 4328 (4336-8) and -CURRENT shows 4360 without VNET. So there's some pretty hefty divergence. Definitely should be using sysctlbyname.

The structure definition is straight from BSD, so part of XNU. 10.whatever-it-is lives here and FreeBSD -CURRENT lives over here with FreeBSD releng/12.1 here. Both KAME 1.46. The "extra" 8 in OSX is icp6s_rfc6980_drop tacked at the end, so I would guess that there's also a map change depending what they added.

akoeplinger commented 4 years ago

I diffed the icmp6.h header file included in Xcode 12 which targets Big Sur with Xcode 11 and there's indeed a new field at the end of the icmp6stat struct:

     u_quad_t icp6s_rfc6980_drop;    /* NDP packet dropped based on RFC 6980 */
+    u_quad_t icp6s_badpkttoobig;    /* bad packet too big */
akoeplinger commented 4 years ago

There's another interesting diff for the tcpstat structure in Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/netinet/tcp_var.h:

@@ -288,44 +288,36 @@
    u_int32_t       tcps_sack_rexmits;          /* SACK rexmit segments   */
    u_int32_t       tcps_sack_rexmit_bytes;     /* SACK rexmit bytes      */
    u_int32_t       tcps_sack_rcv_blocks;       /* SACK blocks (options) received */
    u_int32_t       tcps_sack_send_blocks;      /* SACK blocks (options) sent     */
    u_int32_t       tcps_sack_sboverflow;       /* SACK sendblock overflow   */

    u_int32_t       tcps_bg_rcvtotal;       /* total background packets received */
    u_int32_t       tcps_rxtfindrop;        /* drop conn after retransmitting FIN */
    u_int32_t       tcps_fcholdpacket;      /* packets withheld because of flow control */

-   /* LRO related stats */
-   u_int32_t       tcps_coalesced_pack;    /* number of coalesced packets */
-   u_int32_t       tcps_flowtbl_full;      /* times flow table was full */
-   u_int32_t       tcps_flowtbl_collision; /* collisions in flow tbl */
-   u_int32_t       tcps_lro_twopack;       /* 2 packets coalesced */
-   u_int32_t       tcps_lro_multpack;      /* 3 or 4 pkts coalesced */
-   u_int32_t       tcps_lro_largepack;     /* 5 or more pkts coalesced */
-
    u_int32_t       tcps_limited_txt;       /* Limited transmit used */
    u_int32_t       tcps_early_rexmt;       /* Early retransmit used */
    u_int32_t       tcps_sack_ackadv;       /* Cumulative ack advanced along with sack */

    /* Checksum related stats */
    u_int32_t       tcps_rcv_swcsum;        /* tcp swcksum (inbound), packets */
    u_int32_t       tcps_rcv_swcsum_bytes;  /* tcp swcksum (inbound), bytes */
    u_int32_t       tcps_rcv6_swcsum;       /* tcp6 swcksum (inbound), packets */
    u_int32_t       tcps_rcv6_swcsum_bytes; /* tcp6 swcksum (inbound), bytes */
    u_int32_t       tcps_snd_swcsum;        /* tcp swcksum (outbound), packets */
    u_int32_t       tcps_snd_swcsum_bytes;  /* tcp swcksum (outbound), bytes */
    u_int32_t       tcps_snd6_swcsum;       /* tcp6 swcksum (outbound), packets */
    u_int32_t       tcps_snd6_swcsum_bytes; /* tcp6 swcksum (outbound), bytes */
-   u_int32_t       tcps_msg_unopkts;       /* unordered packet on TCP msg stream */
-   u_int32_t       tcps_msg_unoappendfail; /* failed to append unordered pkt */
-   u_int32_t       tcps_msg_sndwaithipri; /* send is waiting for high priority data */
+   u_int32_t       tcps_unused_1;
+   u_int32_t       tcps_unused_2;
+   u_int32_t       tcps_unused_3;

Looks like they removed the LRO related stats fields without adding placeholders which looks like a bug to me?

rootwyrm commented 4 years ago

Most of the tcpstat use appears to be of the tcpstat.tcps_sndrexmitpack style, but definitely a quick pass should be made. Really, nothing should rely on location in any of the stat blobs and instead access as:

int tcpstat = sysctlbyname("net.inet.tcp.stats", &tcpstat, &stat_len, NULL, 0);
printf("your message here %ju\n", tcpstat.tcps_rxtfindrop);
karelz commented 4 years ago

Reopening to wait on backport into 5.0 in #41370

wfurt commented 4 years ago

Fixed in 6.0/master (PR #41179) and in 5.0 (PR #41370).