Skip to content

Conversation

devnexen
Copy link
Member

With this, connections attempts when ip address and port already bound prior will be refused, thus the behaviod will be closer to unixes ; only little difference tough once the server shutdown the said binding might still be made unavailable for future connections for a little undetermined time.

@devnexen
Copy link
Member Author

I put it on 8.1 because malicious code can use this "ability" to interfere, even tough this sapi cli is "just" a server for dev, it does not kill. But if refused I ll apply the patch from 8.2 no pb.

@devnexen devnexen marked this pull request as ready for review December 26, 2023 09:05
With this, connections attempts when ip address and port are already bound
prior will be refused, thus the behavior will be closer to unixes ;
only little difference tough once the server shutdown the said binding
might still be made unavailable for future connections for a little
undetermined time.
@devnexen devnexen force-pushed the windows_sapi_cli_ipport_fix branch from 1184cb0 to 9c4f1fb Compare December 26, 2023 09:06
@nielsdos
Copy link
Member

I put it on 8.1 because malicious code can use this "ability" to interfere, even tough this sapi cli is "just" a server for dev, it does not kill.

I don't think it fits in the attack model that is used for security issues.

Anyway, looks like SO_REUSEADDR is used on *nix:

#ifdef SO_REUSEADDR
{
int val = 1;
setsockopt(retval, SOL_SOCKET, SO_REUSEADDR, (char*)&val, sizeof(val));
}
#endif

That was introduced in the commit that introduced the CLI server: 5b921a8, RFC: https://wiki.php.net/rfc/builtinwebserver The RFC does not contain many details at all.

I didn't really dig into that much, but if I had to take a guess it may be because the CLI server allows multiple workers on *nix. On Windows, such a mode does not exist (yet). So perhaps the fact that addresses can be reused on Windows was an oversight. Although I'd be interested in the backstory

@mvorisek
Copy link
Contributor

I am the issue reporter and I would like to get it closed.

Can someone review this PR?

@nielsdos
Copy link
Member

I'll check this in the evening, but I would retarget to 8.3, probably a bit risky for 8.2 given we tag the last 8.2 bugfix release early next week

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this and tested this on a Windows machine, and I was still able to start two CLI servers on the same IP+port combination

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

Successfully merging this pull request may close these issues.

3 participants