Re: [OLE #94] Correctly marshal NULL ppUnk refptrs in NdrPointerMarshall

[Robert Shearman]

Mike Hearn wrote:
> This corrects an attempt to demarshal garbage during InstallShield10
> startup. For full InstallShield 10 support we need a stdole2.tlb
>
> Mike Hearn <mh(a)codeweavers.com>
> Correctly marshal NULL ppUnk refptrs in NdrPointerMarshall
>
> Index: dlls/rpcrt4/ndr_marshall.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/rpcrt4/ndr_marshall.c,v
> retrieving revision 1.24
> diff -u -p -d -u -r1.24 ndr_marshall.c
> --- dlls/rpcrt4/ndr_marshall.c	1 Jun 2005 11:04:03 -0000	1.24
> +++ dlls/rpcrt4/ndr_marshall.c	5 Jun 2005 22:03:11 -0000
> @@ -575,19 +575,19 @@ void WINAPI PointerMarshall(PMIDL_STUB_M
>   TRACE("(%p,%p,%p,%p)\n", pStubMsg, Buffer, Pointer, pFormat);
>   TRACE("type=0x%x, attr=", type); dump_pointer_attr(attr);
>   pFormat += 2;
> +  
>   if (attr & RPC_FC_P_SIMPLEPOINTER) desc = pFormat;
>   else desc = pFormat + *(const SHORT*)pFormat;
> +  
>   if (attr & RPC_FC_P_DEREF) {
> +    if (!Pointer)
> +      RpcRaiseException(RPC_X_NULL_REF_POINTER);    
>     Pointer = *(unsigned char**)Pointer;
>     TRACE("deref => %p\n", Pointer);
>   }
>
> switch (type) {
> -  case RPC_FC_RP: /* ref pointer (always non-null) */
> -#if 0 /* this causes problems for InstallShield so is disabled - we need more tests */
> -    if (!Pointer)
> -      RpcRaiseException(RPC_X_NULL_REF_POINTER);
> -#endif
> +  case RPC_FC_RP: /* ref pointer (always non-null but may point to null) */
>     break;
>   case RPC_FC_UP: /* unique pointer */
>   case RPC_FC_OP: /* object pointer - same as unique here */

This looks wrong. A ref pointer shouldn't be treated as a unique pointer 
in any circumstances AFAIK. I'll add this case to my mini test suite to 
confirm or deny this hypothesis.

-- 
Rob Shearman

[OLE #94] Correctly marshal NULL ppUnk refptrs in NdrPointerMarshall

[Mike Hearn]

On Mon, 2005-06-06 at 10:26 -0500, Robert Shearman wrote:
> > switch (type) {
> > -  case RPC_FC_RP: /* ref pointer (always non-null) */
> > -#if 0 /* this causes problems for InstallShield so is disabled - we
> > need more tests */
> > -    if (!Pointer)
> > -      RpcRaiseException(RPC_X_NULL_REF_POINTER);
> > -#endif
> > +  case RPC_FC_RP: /* ref pointer (always non-null but may point to
> > null) */
> >     break;
> >   case RPC_FC_UP: /* unique pointer */
> >   case RPC_FC_OP: /* object pointer - same as unique here */
>
> This looks wrong. A ref pointer shouldn't be treated as a unique
> pointer in any circumstances AFAIK. I'll add this case to my mini test
> suite to confirm or deny this hypothesis.

This is wire-sizing, the full code is:

  switch (type) {
  case RPC_FC_RP:
  case RPC_FC_OP:
  case RPC_FC_UP:
    pStubMsg->BufferLength += 4;
    /* NULL pointer has no further representation */
    if (!Pointer)
        return;
    break;
  case RPC_FC_FP:
  default:
    FIXME("unhandled ptr type=%02x\n", type);
    RpcRaiseException(RPC_X_BAD_STUB_DATA);
  }

  m = NdrBufferSizer[*desc & NDR_TABLE_MASK];
  if (m) m(pStubMsg, Pointer, desc);
  else FIXME("no buffersizer for data type=%02x\n", *desc);

In this case, we need to reserve space for a refptr on the wire to be
able to tell the difference between NULL and non-NULL. So it reserves 4
bytes in the buffer.

[OLE #94] Correctly marshal NULL ppUnk refptrs in NdrPointerMarshall

[Robert Shearman]

Mike Hearn wrote:
> On Mon, 2005-06-06 at 10:26 -0500, Robert Shearman wrote:
> > > switch (type) {
> > > -  case RPC_FC_RP: /* ref pointer (always non-null) */
> > > -#if 0 /* this causes problems for InstallShield so is disabled - we
> >
> > need more tests */
> > > -    if (!Pointer)
> > > -      RpcRaiseException(RPC_X_NULL_REF_POINTER);
> > > -#endif
> > > +  case RPC_FC_RP: /* ref pointer (always non-null but may point to
> >
> > null) */
> > > break;
> > >  case RPC_FC_UP: /* unique pointer */
> > >  case RPC_FC_OP: /* object pointer - same as unique here */
> >
> > This looks wrong. A ref pointer shouldn't be treated as a unique
> > pointer in any circumstances AFAIK. I'll add this case to my mini test
> > suite to confirm or deny this hypothesis.
>
> This is wire-sizing, the full code is:
>
> switch (type) {
>  case RPC_FC_RP:
>  case RPC_FC_OP:
>  case RPC_FC_UP:
>    pStubMsg->BufferLength += 4;
>    /* NULL pointer has no further representation */
>    if (!Pointer)
>        return;
>    break;
>  case RPC_FC_FP:
>  default:
>    FIXME("unhandled ptr type=%02x\n", type);
>    RpcRaiseException(RPC_X_BAD_STUB_DATA);
>  }
>
> m = NdrBufferSizer[*desc & NDR_TABLE_MASK];
>  if (m) m(pStubMsg, Pointer, desc);
>  else FIXME("no buffersizer for data type=%02x\n", *desc);
>
> In this case, we need to reserve space for a refptr on the wire to be
> able to tell the difference between NULL and non-NULL. So it reserves 4
> bytes in the buffer.

Exactly. A refptr shouldn't have those extra 4 bytes because it should 
never be NULL. I would be very surprised if Microsoft have chosen to be 
inconsistent here.


-- 
Rob Shearman

[OLE #94] Correctly marshal NULL ppUnk refptrs in NdrPointerMarshall

[Mike Hearn]

On Mon, 2005-06-06 at 11:11 -0500, Robert Shearman wrote:
> Exactly. A refptr shouldn't have those extra 4 bytes because it should
> never be NULL. I would be very surprised if Microsoft have chosen to
> be inconsistent here.

Refptrs can't be NULL but their contents once de-referenced can be, the
problem here is that we have a function like this:

Foo([in] ?? a, [in] ?? b, [out] IBar **bar);

bar is a refptr, it's not NULL. But the contents once dereferenced are
(ie, it's returning a null ifptr). We aren't marshalling this correctly
at the moment, instead we don't allocate any space in the buffer for the
refptr itself and just dereference it. Then when we discover it's NULL,
we don't marshal anything (because there's nothing to demarshal) so we
lose sync on the receiving end.

thanks -mike