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