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.

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.