Re: [PATCH] win32 - dropped data in read_with_timeout

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

This patch looks correct, and because of the nature of the flaw it fixes,
this is issue 2/2 that held me up this weekend.

The reason I've not committed your patch verbatim, is that I'm trying to
grok the expected and unexpected result codes that arise in this situation.

You also ignore the result of the CancelIo() call which troubles me, we
should be able to reduce that result to one of two expected values as well.

Bill

Norris Leong wrote:
> Hi All,
> 
> I suspect that read_with_timeout (in file_io/win32/readwrite.c) may drop 
> data if
> 
> (i) WaitForSingleObject (line 82) returns WAIT_TIMEOUT, and
> (ii) Data arrived before the execution arrives at CancelIo (line 105).
> 
> Tried the attached patch and the problem seems solved.  I'm a novice of 
> Windows' file mgmt API and would appreciate if somebody can validate 
> this patch.
> 
> Regards,
> Norris Leong
> 
> 
> ------------------------------------------------------------------------
> 
> diff -ruN apr-1.2.2.orig/file_io/win32/readwrite.c apr-1.2.2/file_io/win32/readwrite.c
> --- apr-1.2.2.orig/file_io/win32/readwrite.c	2006-01-20 11:46:52.000000000 +0800
> +++ apr-1.2.2/file_io/win32/readwrite.c	2006-01-20 11:41:13.000000000 +0800
> @@ -102,7 +102,12 @@
>              }
>              if (rv != APR_SUCCESS) {
>                  if (apr_os_level >= APR_WIN_98)
> +                {
>                      CancelIo(file->filehand);
> +                    if (GetOverlappedResult(file->filehand, file->pOverlapped, 
> +                                        (LPDWORD)nbytes, TRUE))
> +                        rv = APR_SUCCESS;
> +                }
>              }
>          }
>          else if (rv == APR_FROM_OS_ERROR(ERROR_BROKEN_PIPE)) {

Re: [PATCH] win32 - dropped data in read_with_timeout

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

William A. Rowe, Jr. wrote:
> This patch looks correct, and because of the nature of the flaw it fixes,
> this is issue 2/2 that held me up this weekend.

FYI - this is considered a non-issue by our 1.2 RM, so I am not addressing
it in the current release cycle as it's more important to tag a 0.9 release
matching the current 1.2, and it's something to address in the next cycle
when we also catch the WAIT_ABANDONED case.

Note that I'm still stuck trying to envision a current use case for this
exception in any Apache code.  The WAIT_ABANDONED case is not an issue in
correctly written code, since the the thread holding a lock should -not-
be abruptly terminated in any normal case.  The TIMEOUT case is not an issue
if there is a timeout, since traditional network apps will abandon the
client / server when it hasn't responded in the specified TIMEOUT.  The
only case I can envison where this particular fix is effective, is in the
case of a nonblocking ping, timeout 0, looking to see if the channel is
ready.  In that edge case, I can see data arriving in the small window
before we give up.

Note also this is not the only occurance in the code of this condition,
and it is interrelated to the WAIT_ABANONDED case, which is why the correct
patch is far more complex.

Let us know if we've misunderstood your expected cases.

Bill