It looks reasonable. Would you be able to add a test for this? And pls could you open a bug?

I also checked the code a little bit and it seems that it might also overflow in socket_sendto fom long paths on mac in here:

https://github.com/mmeyer724/php-src/blob/1f7ee2f69591abec1c224ae75f2a750ef860bc4f/ext/sockets/sockets.c#L1926

I might be wrong but it seems that sun_path is hard coded to 108 and the osx limit is 104: https://developer.apple.com/documentation/kernel/sockaddr_un/1541426-sun_path?language=objc

Anyway it's a different issue so not really related to this one...

@mmeyer724 any action here ?

@krakjoe Sorry I forgot about this! I'm working on bukka's feedback now

@bukka Sorry for the delay, would you mind checking out the .phpt test I just pushed? :-)
Link to the bug I created: https://bugs.php.net/bug.php?id=76839

For what it's worth, I'm pretty sure the appveyor build failures are unrelated to my changes here. 😕

LGTM

What is the reason that Windows is skipped here? I'm not too familiar with the socket code but I did not see any obvious places for anything not implemented on Windows in the surrounding blocks?

@KalleZ I initially wrote the test without the windows exclusion, but it failed hard in the CI pipeline. As far as I can tell AF_UNIX sockets are not supported on Windows (or at least they're not supported on the version of windows appveyor is using: https://blogs.msdn.microsoft.com/commandline/2017/12/19/af_unix-comes-to-windows)

Aha thanks, that clears it up a little! I was just curious given we don't have any surrounding guards for the AF_UNIX blocks in the code. Perhaps thats something we should look into, but that can wait for another issue.

Alternatively you can use the userland constants ''PHP_WINDOWS_XXX'' to see if the version of Windows supports like:

if(PHP_OS_FAMILY == 'Windows' && (PHP_WINDOWS_VERSION_MAJOR < 10 || (PHP_VERSION_MAJOR === 10 && PHP_VERSION_BUILD < 17063))) { die('skip: This test requires Windows 10, build 17063 or greater'); }

(Keep in mind its just a pseudo)

For what i'd care, this patch is fine. Unix socket supprot on Windows needs to be yet revisited and enabled/completed including tests. The patch seems to be fine for now for 7.2 as well.

Thanks.

Merged as 3c42c78.

Thanks