[PATCH 3/3] rpcrt4: Don't free server-allocated memory with the MustFree flag.

[Zebediah Figura]

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 <z.figura12 at gmail.com>
>>> ---
>>>  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.