Re: rpcrt4: Fix regression in NdrConformantStringUnmarshall by doing the memcpy check in safe_copy_from_buffer

[Maarten Lankhorst]

Hi Rob,

Robert Shearman schreef:
> Maarten Lankhorst wrote:
> > @@ -665,7 +665,8 @@ static inline void
> > safe_copy_from_buffer(MIDL_STUB_MESSAGE *pStubMsg, void *p, U
> >      if ((pStubMsg->Buffer + size < pStubMsg->Buffer) || /* integer
> > overflow of pStubMsg->Buffer */
> >          (pStubMsg->Buffer + size > pStubMsg->BufferEnd))
> >          RpcRaiseException(RPC_X_BAD_STUB_DATA);
> > - memcpy(p, pStubMsg->Buffer, size);
> > - if (p != pStubMsg->Buffer)
> > -
> >    memcpy(p, pStubMsg->Buffer, size);
> > pStubMsg->Buffer += size;
> >
> > }
> >
> > @@ -890,8 +891,7 @@ unsigned char *WINAPI
> > NdrConformantStringUnmarshall( PMIDL_STUB_MESSAGE pStubMsg
> >        *ppMemory = NdrAllocate(pStubMsg, memsize);
> >    }
> > - if (*ppMemory != pStubMsg->Buffer)
> > - safe_copy_from_buffer(pStubMsg, *ppMemory, bufsize);
> > -
> >
> > safe_copy_from_buffer(pStubMsg, *ppMemory, bufsize);
> >
> > if (*pFormat == RPC_FC_C_CSTRING) {
> >   TRACE("string=%s\n", debugstr_a((char*)*ppMemory));
>
> Good work in spotting and fixing the mistake I made, but I think I'd
> prefer to fix it by making the caller of safe_copy_from_buffer do the
> incrementing of the buffer. This is to avoid confusion with the name
> of the function and to avoid the possibility that the buffer is
> incremented twice.

If you want to remove that, you might as well remove that whole inline,
since even when the two areas are equal the check that needs to be
performed at overflowing still needs to be done even if the areas are
the same.
Maybe just call it safely_append_from_buffer instead?

Cheers,
Maarten.