Fix ICMP socket leak in hs.ping

[cmsj]

No description provided.

[github-actions]

View Test Results

348 tests   296 ✔️  11m 44s ⏱️
    2 suites    52 💤
    1 files        0 ❌

Results for commit f30c5c0.

[asmagill]

I'll try and take a look at it tomorrow (later today I guess at this point...)

[cmsj]

@asmagill thanks! There's a bit of commentary in the related bug, but the tl;dr is that kCFSocketCloseOnInvalidate, while set by default (and assert()ed as present) when the socket is created, is later unset in - (void)simplePing:(SimplePing *)pinger didStartWithAddress:(NSData *)address so the underlying native socket is never closed.

At least that is true for the simple case of calling hs.network.ping() in a one-shot style. I'm wondering if there's something I've missed for more complex cases, but even so I think this patch should be safe in any case, because it's in the path of CFSocketInvalidate() so there should be no reason for the underlying native socket to exist beyond that point.

[asmagill]

Yeah, your logic tracks and the code looks good.

I'm a little curious as to why the flag is removed in the simplePing method -- I'll try and dig into the actual SimplePing code this weekend and see if I can figure out why and if we're still open to issues in more complex use cases, but from reading the CFSocket stuff in Apple's docs, I agree that this added check is needed.

[cmsj]

Thanks!