Re: [PATCH] rpcrt4: wait_async_request: return error if we received an error
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
Re: [PATCH] rpcrt4: wait_async_request: return error if we received an error
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
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
Re: [PATCH] rpcrt4: wait_async_request: return error if we received an error
Hi Marcus,
On 09/29/12 11:44, 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.
Agreed, thanks for catching it.
Ciao, Marcus
---
dlls/rpcrt4/rpc_transport.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/dlls/rpcrt4/rpc_transport.c b/dlls/rpcrt4/rpc_transport.c
index 686ce68..e572b78 100644
--- a/dlls/rpcrt4/rpc_transport.c
+++ b/dlls/rpcrt4/rpc_transport.c
@@ -1882,7 +1882,7 @@ static RPC_STATUS wait_async_request(RpcHttpAsyncData
*async_data, BOOL call_ret
if(call_ret) {
RpcHttpAsyncData_Release(async_data);
-return RPC_S_OK;
+return call_ret; /* The Http* status codes map into the RPC_S
namespace */
Not really, Http* APIs return BOOL and GetLastError() call is needed to
get error code. AFAIR, when I tested this code, most HTTP-related errors
were translated to RPC_S_SERVER_UNAVAILABLE by rpcrt4. That may be a
better choice here as well.
Jacek
