Re: [PATCH] rpcrt4: wait_async_request: return error if we received an error
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? Jacek
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
On 10/03/12 14:57, Marcus Meissner wrote:
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
I restored my login in Coverity and looked at the report. It seems to be false positive. async_data is reference counted structure and we'll never free it in mentioned wait_async_request because we still store the reference in caller. Cheers, Jacek
participants (2)
-
Jacek Caban -
Marcus Meissner