Re: [PATCH] wininet: Close sockets open in http requests

[Misha Koshelev]

On Wed, 2007-10-17 at 17:10 -0700, Nigel Liang wrote:
> On 10/17/07, Misha Koshelev  wrote:
> > On Wed, 2007-10-17 at 17:10 -0500, Misha Koshelev wrote:
> > > > n 10/13/07, Nigel Liang <ncliang at gmail.com> wrote:
> > > > > Hi,
> > > > >
> > > > > Http requests open sockets and forget to close them. Left running over a long
> > > > > period, the socket file descriptor starts getting large. Once it exceeds
> > > > > FD_SETSIZE, the behavior becomes unpredictable and results in funcky crashes.
> > > > >
> > > > > -Nigel
> > > > >
> > > > > dlls/wininet/internet.c |    3 +++
> > > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/dlls/wininet/internet.c b/dlls/wininet/internet.c
> > > > > index 0edca74..8e3a98e 100644
> > > > > --- a/dlls/wininet/internet.c
> > > > > +++ b/dlls/wininet/internet.c
> > > > > @@ -1014,6 +1014,9 @@ BOOL WINAPI InternetCloseHandle(HINTERNE
> > > > >          return FALSE;
> > > > >      }
> > > > > - if (lpwh->htype == WH_HHTTPREQ)
> > > > > -
> > > > >    NETCON_close(&((LPWININETHTTPREQW)lpwh)->netConnection);
> > > > > - WININET_Release( lpwh );
> > > > >    WININET_FreeHandle( hInternet );
> > > > >
> > > > > --
> > > > > 1.4.1
> > > >
> > > > Hi,
> > > >
> > > > This patch was sent 4 days ago and it hasn't gone in yet. Could anyone
> > > > give me some comments on it? Dan filed bug 10032 which is related to
> > > > this. While we shouldn't be using select() on high FDs for networking,
> > > > this patch closes the sockets that were open by http requests to avoid
> > > > getting to high FDs...
> > > >
> > > > Thanks,
> > > > -Nigel
> > >
> > > Hi, well looking over this very quickly right off the bat it does not seem
> > > correct to me. WININET_Release will call the close_connection method of the
> > > handle if it is defined, and for WH_HHTTPREQ this is HTTP_CloseConnection,
> > > which does call NETCON_Close along with sending all the proper status notifications,
> > > etc.
> > >
> > > So if in fact NETCON_Close is _not_ being called for some reason, you need to track
> > > down where involving those functions either why the proper one is not
> > > getting called (if that is the case) or if that function somehow needs to call
> > > NETCON_Close when it doesn't.
> > >
> > > Misha
> >
> > Actually, thinking about this a little more, if this is indeed happening
> > (NETCON_close not getting called) then likely this is a reference count
> > problem for the object, and thus the proper fix would be to find where
> > the reference count is being incremented but not appropriately
> > decremented, as info->close_connection only gets called if the reference
> > count of the object is zero. In my own experience, there have been quite
> > a few places where reference counts were incremented and not decremented
> > appropriately in wininet (the ones I ran into I believe are patched now
> > though).
>
> Hi,
>
> Yep, you were right. There was a missing call to WININET_Release.
> Found it and submitted a new patch:
> http://www.winehq.org/pipermail/wine-patches/2007-October/045337.html
>
> Thanks!
> -Nigel

Glad my hours of staring at wininet led to something useful :) I looked
at your new patch and it looks good to me. Now if I could just figure
out bug 9922 (or even knew if it still occurs since I can't reproduce
it)...

Misha