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

[Robert Shearman]

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.

-- 
Rob Shearman

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.

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

[Robert Shearman]

Maarten Lankhorst wrote:
> 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;
> > >  }
> >
> > 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.

That's what safe_buffer_increment does.

-- 
Rob Shearman