Re: [PATCH 5/6] ws2_32: Added support for overlapped GetAddrInfoExW.

[Sebastian Lackner]

On 01.08.2017 18:29, Jacek Caban wrote:
> +static void WINAPI getaddrinfo_callback(TP_CALLBACK_INSTANCE *instance, void *context, TP_WORK *work)
> +{
> - struct getaddrinfo_args *args = context;
> - OVERLAPPED *overlapped = args->overlapped;
> - struct WS_addrinfo *res;
> -
> - overlapped->Internal = WS_getaddrinfo(args->nodename, args->servname, NULL, &res);

What if the app does busy-looping instead of using the event, and expects the result to
be set after overlapped->Internal changed?

My understanding is that setting overlapped->Internal should be the very last thing in
the function, the app could decide to deallocate / reuse the memory after the async
operation has finished.
> - if (res)
> - {
> -
>    *args->result = addrinfo_list_AtoW(res);
> -
>    overlapped->u.Pointer = args->result;
> -
>    WS_freeaddrinfo(res);
> - }
>
> [...]
> -
>    tp_work = CreateThreadpoolWork(getaddrinfo_callback, args, NULL);
> -
>    if (!tp_work)
> -
>    {
> -
>        HeapFree(GetProcessHeap(), 0, args);
> -
>        ret = GetLastError();
> -
>        goto end;
> -
>    }
>
> -
>    overlapped->Internal = WSAEINPROGRESS;
> -
>    SubmitThreadpoolWork(tp_work);

I don't see a CloseThreadpoolWork call or cleanup group anywhere, so there is likely a
leak in this code. Also, when all callbacks have different arguments, it would be better
to use TrySubmitThreadpoolCallback(). Work callbacks are typically used when the same
task is queued multiple times.
> -
>    if (local_nodenameW != nodename)
> -
>        HeapFree(GetProcessHeap(), 0, local_nodenameW);
> -
>    WSASetLastError(ERROR_IO_PENDING);
> -
>    return ERROR_IO_PENDING;
> - }
> -
> - ret = WS_getaddrinfo(nodenameA, servnameA, hints, &resA);

[PATCH 5/6] ws2_32: Added support for overlapped GetAddrInfoExW.

[Jacek Caban]

On 01.08.2017 18:52, Sebastian Lackner wrote:
> On 01.08.2017 18:29, Jacek Caban wrote:
> > +static void WINAPI getaddrinfo_callback(TP_CALLBACK_INSTANCE *instance, void *context, TP_WORK *work)
> > +{
> > - struct getaddrinfo_args *args = context;
> > - OVERLAPPED *overlapped = args->overlapped;
> > - struct WS_addrinfo *res;
> > -
> > - overlapped->Internal = WS_getaddrinfo(args->nodename, args->servname, NULL, &res);
>
> What if the app does busy-looping instead of using the event, and expects the result to
> be set after overlapped->Internal changed?
>
> My understanding is that setting overlapped->Internal should be the very last thing in
> the function, the app could decide to deallocate / reuse the memory after the async
> operation has finished.

It's not clear for me if it's supposed to be guaranteed, but it surely
won't hurt. I will change it.
> > - if (res)
> > - {
> > -
> >    *args->result = addrinfo_list_AtoW(res);
> > -
> >    overlapped->u.Pointer = args->result;
> > -
> >    WS_freeaddrinfo(res);
> > - }
> >
> > [...]
> > -
> >    tp_work = CreateThreadpoolWork(getaddrinfo_callback, args, NULL);
> > -
> >    if (!tp_work)
> > -
> >    {
> > -
> >        HeapFree(GetProcessHeap(), 0, args);
> > -
> >        ret = GetLastError();
> > -
> >        goto end;
> > -
> >    }
> >
> > -
> >    overlapped->Internal = WSAEINPROGRESS;
> > -
> >    SubmitThreadpoolWork(tp_work);
>
> I don't see a CloseThreadpoolWork call or cleanup group anywhere, so there is likely a
> leak in this code. Also, when all callbacks have different arguments, it would be better
> to use TrySubmitThreadpoolCallback(). Work callbacks are typically used when the same
> task is queued multiple times.

TrySubmitThreadpoolCallback() looks like what I need, I will change it.

Thanks,
Jacek