Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

Crossposting to dev@apr. Maybe some Windows folks want to feedback.

Regards

Rüdiger

On 6/20/24 12:12 PM, Ruediger Pluem wrote:
> 
> 
> On 6/19/24 2:52 PM, Joe Orton wrote:
>> On Wed, Jun 19, 2024 at 02:04:20PM +0200, Yann Ylavic wrote:
>>> On Wed, Jun 19, 2024 at 1:00 PM Ruediger Pluem <[email protected]> wrote:
>>>> As far as I read the code it does not.
>>>>
>>>> https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433
>>>>
>>>> We copy the data (sa, salen family and port) not a pointer.
>>>
>>> Ah yes, I was looking at win32 code, while Joe fixed it 13 years ago
>>> for unix (r983618).
>>> So the pointer copy exists, but only for WIN32 and OS/2 AFAICT, what a mess.
>>
>> Sorry!
>>
>>> Let me fix that then ;)
>>
>> There is a test in that commit, is it not catching the bug on non-Unix?
> 
> As far as I can tell the test is never hit on Windows as apr_socket_connect(cd, sa) returns APR_SUCCESS
> despite being non blocking.
> 
> https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/test/testsock.c#L428-L435
> 
> .\testall.exe -v testsock
> testsock            : |Line 433: Cannot test if connect completes synchronously
> SUCCESS
> All tests passed.
> 
> The following patch would fix this by causing the test to executed and of course to fail:
> 
> Index: network_io/win32/sockets.c
> ===================================================================
> --- network_io/win32/sockets.c	(revision 1917061)
> +++ network_io/win32/sockets.c	(working copy)
> @@ -378,7 +378,7 @@
>      }
> 
>      if (rv == APR_FROM_OS_ERROR(WSAEWOULDBLOCK)) {
> -        if (sock->timeout == 0) {
> +        if (sock->timeout <= 0) {
>              /* Tell the app that the connect is in progress...
>               * Gotta play some games here.  connect on Unix will return
>               * EINPROGRESS under the same circumstances that Windows
> 
> 
> 
> But as Windows is not my native environment I would appreciate remote eyes if the fix is correct.
> 
>