From: Zebediah Figura Subject: Re: [PATCH 3/3] rpcrt4: Don't free server-allocated memory with the MustFree flag. Message-Id: <259baae8-6e10-a468-b8ea-63c89b2242cd@gmail.com> Date: Mon, 15 Oct 2018 13:13:51 -0500 In-Reply-To: References: <1539470804-32048-1-git-send-email-z.figura12@gmail.com> <1539470804-32048-3-git-send-email-z.figura12@gmail.com> <20181015091852.GN31910@merlot.physics.ox.ac.uk> On 15/10/18 12:39, Zebediah Figura wrote: > On 15/10/18 04:18, Huw Davies wrote: >> On Sat, Oct 13, 2018 at 05:46:44PM -0500, Zebediah Figura wrote: >>> Since it will have already been freed in the MUSTFREE pass. >>> >>> Signed-off-by: Zebediah Figura >>> --- >>> dlls/rpcrt4/ndr_stubless.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/dlls/rpcrt4/ndr_stubless.c b/dlls/rpcrt4/ndr_stubless.c >>> index c2f260c..768bdc1 100644 >>> --- a/dlls/rpcrt4/ndr_stubless.c >>> +++ b/dlls/rpcrt4/ndr_stubless.c >>> @@ -1215,7 +1215,7 @@ static LONG_PTR *stub_do_args(MIDL_STUB_MESSAGE *pStubMsg, >>> } >>> break; >>> case STUBLESS_FREE: >>> - if (params[i].attr.ServerAllocSize) >>> + if (params[i].attr.ServerAllocSize && !params[i].attr.MustFree) >>> { >>> HeapFree(GetProcessHeap(), 0, *(void **)pArg); >>> } >> >> Do we need to free in the IsSimpleRef case (like the else block >> below)? In this case the freer won't see the top-level ptr, though >> it may not be an issue with ServerAllocSize. >> >> Huw. >> >> > > Hmm, no, you're right, we do need to free in that case. So I think the > condition should be (ServerAllocSize && (!MustFree || IsSimpleRef)). > > What confuses me about the whole ordeal is that in theory, > ServerAllocSize represents storage space that should exist on the stub's > stack. MSDN states this: > > "The ServerAllocSize bits are nonzero if the parameter is [out], [in], > or [in,out] pointer to pointer, or pointer to enum16, and will be > initialized on the server interpreter's stack, rather than using a call > to I_RpcAllocate." > > and -Os mode stubs back this up — but only in some cases. Namely, a > parameter like "[out] mystruct_t *x" will have a server size of 16 bits, > and the inline stub will include a generated "MYSTRUCT _xM" which the > server function is called with. However, in the case that's causing the > crash for me, i.e. where IsSimpleRef is *not* set, e.g. any double > pointer, the -Os mode stubs don't allocate stack space for the inner > pointer, and just calling the type's freer — i.e. NdrPointerFree() — > would incorrectly free that stack space if it were allocated. The > effect, then, is that ServerAllocSize seems to have to be entirely > ignored if IsSimpleRef isn't also set, which is weird. Am I missing > something here? > > Actually, never mind, I just did a little more testing and found the problem: MIDL will mark these pointers as on-stack if and only if -Oicf mode is used, so they won't be freed by NdrPointerFree(). widl doesn't do that. It's still kind of weird that MIDL doesn't allocate inner pointers on the stack in -Os mode. I guess it's just an optimization they never implemented because they want everyone to use -Oicf mode.