Re: [ros-dev] [ros-diffs] [reactos] 01/01: CID 1206831 Dereference after null check

[Jerome Gardou]

HI,

that doesn't look good, as shown by 
https://reactos.org/testman/compare.php?ids=56275,56276

Jérôme


Le 29/10/2017 à 11:17, Samuel Serapion a écrit :
>  
> https://git.reactos.org/?p=reactos.git;a=commitdiff;h=b3b2a23f05e5188dc1475…
>
>  commit b3b2a23f05e5188dc1475961fcd7f036f0046d25
>  Author: Samuel Serapion <samcharly(a)hotmail.com>
>  AuthorDate: Fri Oct 20 14:00:32 2017 -0400
>
>       CID 1206831 Dereference after null check
>       
>       BytesRead is an optional out parameter and must be checked before being written to.
>  ---
>    sdk/lib/rtl/memstream.c | 3 ++-
>    1 file changed, 2 insertions(+), 1 deletion(-)
>
>  diff --git a/sdk/lib/rtl/memstream.c b/sdk/lib/rtl/memstream.c
>  index 0549424ca4..8fe4169fb1 100644
>  --- a/sdk/lib/rtl/memstream.c
>  +++ b/sdk/lib/rtl/memstream.c
>  @@ -185,7 +185,8 @@ RtlReadMemoryStream(
>    
>        Stream->Current = (PUCHAR)Stream->Current + CopyLength;
>    
>  -    *BytesRead = CopyLength;
>  +    if (BytesRead)
>  +        *BytesRead = CopyLength;
>    
>        return S_OK;
>    }
>

[encoded]

Seems like this API has a 'feature' where by it throws exceptions if BytesRead
is null?

On Sun, Oct 29, 2017 at 8:02 AM, Jerome Gardou <jerome.gardou(a)reactos.org>
wrote:

>   HI,
>
>  that doesn't look good, as shown by https://reactos.org/testman/co
>  mpare.php?ids=56275,56276
>
>  Jérôme
>
>
>  Le 29/10/2017 à 11:17, Samuel Serapion a écrit :
>
> >  
> > https://git.reactos.org/?p=reactos.git;a=commitdiff;h=b3b2a2
> >  3f05e5188dc1475961fcd7f036f0046d25
> >
> >  commit b3b2a23f05e5188dc1475961fcd7f036f0046d25
> >  Author: Samuel Serapion <samcharly(a)hotmail.com>
> >  AuthorDate: Fri Oct 20 14:00:32 2017 -0400
> >
> >       CID 1206831 Dereference after null check
> >            BytesRead is an optional out parameter and must be checked
> >  before being written to.
> >  ---
> >    sdk/lib/rtl/memstream.c | 3 ++-
> >    1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >  diff --git a/sdk/lib/rtl/memstream.c b/sdk/lib/rtl/memstream.c
> >  index 0549424ca4..8fe4169fb1 100644
> >  --- a/sdk/lib/rtl/memstream.c
> >  +++ b/sdk/lib/rtl/memstream.c
> >  @@ -185,7 +185,8 @@ RtlReadMemoryStream(
> >          Stream->Current = (PUCHAR)Stream->Current + CopyLength;
> >    -    *BytesRead = CopyLength;
> >  +    if (BytesRead)
> >  +        *BytesRead = CopyLength;
> >          return S_OK;
> >    }
> >
> >  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Thomas Faber]

Or the annotation is just wrong and this parameter is not optional?


On 2017-10-31 16:07, encoded wrote:
>   Seems like this API has a 'feature' where by
> it throws exceptions if BytesRead
>  is null?
>  
>  On Sun, Oct 29, 2017 at 8:02 AM, Jerome Gardou <jerome.gardou(a)reactos.org>
>  wrote:
>  
> > HI,
> >
> > that doesn't look good, as shown by https://reactos.org/testman/co
> > mpare.php?ids=56275,56276
> >
> > Jérôme
> >
> >
> > Le 29/10/2017 à 11:17, Samuel Serapion a écrit :
> >
> >> https://git.reactos.org/?p=reactos.git;a=commitdiff;h=b3b2a2
> >> 3f05e5188dc1475961fcd7f036f0046d25
> >>
> >> commit b3b2a23f05e5188dc1475961fcd7f036f0046d25
> >> Author: Samuel Serapion <samcharly(a)hotmail.com>
> >> AuthorDate: Fri Oct 20 14:00:32 2017 -0400
> >>
> >>       CID 1206831 Dereference after null check
> >>            BytesRead is an optional out parameter and must be checked
> >> before being written to.
> >> ---
> >>    sdk/lib/rtl/memstream.c | 3 ++-
> >>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/sdk/lib/rtl/memstream.c b/sdk/lib/rtl/memstream.c
> >> index 0549424ca4..8fe4169fb1 100644
> >> --- a/sdk/lib/rtl/memstream.c
> >> +++ b/sdk/lib/rtl/memstream.c
> >> @@ -185,7 +185,8 @@ RtlReadMemoryStream(
> >>          Stream->Current = (PUCHAR)Stream->Current + CopyLength;
> >>    -    *BytesRead = CopyLength;
> >> +    if (BytesRead)
> >> +        *BytesRead = CopyLength;
> >>          return S_OK;
> >>    }
> >>
> >>