[PATCH] rpcrt4: wait_async_request: return error if we received an error

Marcus Meissner marcus at jet.franken.de
Wed Oct 3 07:57:39 CDT 2012


On Tue, Oct 02, 2012 at 01:19:37PM +0200, Jacek Caban wrote:
> Hi Marcus,
> 
> On 10/01/12 23:00, Marcus Meissner wrote:
> > Hi,
> >
> > Various coverity issues complain about user-after-free scenarios,
> > all involving this code path.
> >
> > I strongly think if call_ret signals error, we also need to return
> > an error condition to avoid the callers from proceeding as if nothing
> > happened.
> >
> > Second reiteration with Jaceks comment applied
> 
> Sorry for not catching this later, I was concentrated on your comment
> instead of the code, but the important thing is that true call_ret means
> success (and wininet doing request asynchronously is signalled as an
> error). It means that in this case we want to return RPC_S_OK. What is
> the exact problem?

Ok, my fix was likely bad.

Coverity is spotting use-after-free scenarios.

CID 715805

rpcrt4_http_prepare_in_pipe()

    /* prepare in pipe */
    status = send_echo_request(in_request, async_data, cancel_event);
    if (status != RPC_S_OK) return status;

... here it thinks async_data might be returned freed in the non-async case ... 

    memset(&buffers_in, 0, sizeof(buffers_in));
    buffers_in.dwStructSize = sizeof(buffers_in);
    /* FIXME: get this from the registry */
    buffers_in.dwBufferTotal = 1024 * 1024 * 1024; /* 1Gb */
    prepare_async_request(async_data);

... but it is again referenced here ... 

    ret = HttpSendRequestExW(in_request, &buffers_in, NULL, 0, 0);
    status = wait_async_request(async_data, ret, cancel_event);
    if (status != RPC_S_OK) return status;


send_echo_request() looks like:
static RPC_STATUS send_echo_request(HINTERNET req, RpcHttpAsyncData *async_data, HANDLE cancel_event)
{
    DWORD bytes_read;
    BYTE buf[20];
    BOOL ret;
    RPC_STATUS status;

    prepare_async_request(async_data);
    ret = HttpSendRequestW(req, NULL, 0, NULL, 0);
    status = wait_async_request(async_data, ret, cancel_event);
    if (status != RPC_S_OK) return status;

    status = rpcrt4_http_check_response(req);
    if (status != RPC_S_OK) return status;

    InternetReadFile(req, buf, sizeof(buf), &bytes_read);
    /* FIXME: do something with retrieved data */

    return RPC_S_OK;
}

so with the bumped reference count via prepare_async_request I think it might be safe here.

(and so is a coverity misdetection I think now)

Ciao, Marcus



More information about the wine-devel mailing list