CR-Client Bug 4914MP4LATMDepack::GetPayloadLengths -m_pSlotLengths computation is not saf

This updated change looks good to me.

Eric

> Hi Eric,
> Thanks for your comments.
>
> Please find updated cvs diff below :
>
> Index: mp4-latm-depack.cpp
>
============================================================
=======
> RCS file:
/cvsroot/datatype/mp4/payload/mp4-latm-depack.cpp,v
> retrieving revision 1.5.30.2
> diff -u -w -r1.5.30.2 mp4-latm-depack.cpp
> --- mp4-latm-depack.cpp 4 Jan 2006 06:55:11 -0000      
1.5.30.2
> +++ mp4-latm-depack.cpp 27 Jun 2006 11:58:12 -0000
>  -201,7 +201,10 
>
>          if (bMarker)
>          {
> -           ProcessStagingBuffer(pBuffer,ulSize);
> +           if (!ProcessStagingBuffer(pBuffer,ulSize))
> +            {
> +                failed = TRUE;
> +            }
>          }
>       }
>       else if (bMarker)
>  -261,7 +264,11 
>
>              ulFrameTime += (i * m_ulFrameDuration);
>
> -           GetPayloadLengths(bs);
> +           if (!GetPayloadLengths(bs))
> +            {
> +                failed = TRUE;
> +                break;
> +            }
>              GetPayloads(bs, ulFrameTime);
>          }
>
>  -286,8 +293,9 
>       return ret;
>   }
>
> -void MP4LATMDepack::GetPayloadLengths(Bitstream&
bs)
> +HXBOOL MP4LATMDepack::GetPayloadLengths(Bitstream&
bs)
>   {
> +    HXBOOL failed = FALSE;
>       if (m_muxConfig.AllSameTiming())
>       {
>          for (ULONG32 prog = 0; prog <
m_muxConfig.NumPrograms(); prog++)
>  -308,6 +316,13 
>                          m_pSlotLengths[streamID] +=
tmp;
>                      } while (tmp == 0xff);
>
> +                    if (m_pSlotLengths[streamID] >
bs.GetBufSize())
> +                    {
> +                        failed = TRUE;
> +                        break;
> +                    }
> +
> +
>                      // Resize the payload buffer if we
encounter a
> payload
>                      // size that is larger than our
buffer
>                      if (m_pSlotLengths[streamID] >
m_ulPayloadBufSize)
>  -321,6 +336,7 
>              }
>          }
>       }
> +    return !failed;
>   }
>
>   void MP4LATMDepack::GetPayloads(Bitstream& bs,
ULONG32 ulTime)
> Index: pub/mp4-latm-depack.h
>
============================================================
=======
> RCS file:
/cvsroot/datatype/mp4/payload/pub/mp4-latm-depack.h,v
> retrieving revision 1.1.12.2
> diff -u -w -r1.1.12.2 mp4-latm-depack.h
> --- pub/mp4-latm-depack.h       4 Jan 2006 07:49:14
-0000       1.1.12.2
> +++ pub/mp4-latm-depack.h       27 Jun 2006 11:58:13
-0000
>  -77,7 +77,7 
>       HXBOOL ProcessStagingBuffer(const UINT8*
pBuffer,ULONG32
> ulMuxConfigSize);
>
>       HXBOOL HandleMuxConfig(Bitstream& bs);
> -    void GetPayloadLengths(Bitstream& bs);
> +    HXBOOL GetPayloadLengths(Bitstream& bs);
>       void GetPayloads(Bitstream& bs, ULONG32
ulTime);
>
>   private:
>
>
> Thanks and regards
> ~Anil Jaiswal
>
> At 05:28 PM 6/27/2006, Anil Jaiswal wrote:
>>At 12:02 AM 6/27/2006, Eric Hyche wrote:
>>>
>>>In that case, seems like you would want to set
>>>
>>>failed = TRUE;
>>>
>>>before returning !failed? As it stands now,
it's
>>>going to return TRUE, which would be the same
>>>thing it returned if the function actually
>>>succeeded, which doesn't seem right.
>>>
>>>Eric
>>>
>>> > -----Original Message-----
>>> > From: Anil Jaiswal [mailto:ajaiswalreal.com]
>>> > Sent: Monday, June 26, 2006 1:36 PM
>>> > To: ehychereal.com
>>> > Cc: datatype-devlists.helixcommunity.org
>>> > Subject: Re: [datatype-dev] RESEND
CR-Client Bug
>>> > 4914MP4LATMDepack::GetPayloadLengths
-m_pSlotLengths
>>> > computation is not safe
>>> >
>>> > Hi Eric,
>>> > Sorry for the confusion.
>>> >
>>> > I followed already existing approach of
returning value of
>>> > "failed" in
::ProcessStagingBuffer()
>>> >
>>> > Please find  ::ProcessStagingBuffer()
definition below:
>>> >
>>> > HXBOOL
MP4LATMDepack::ProcessStagingBuffer(const UINT8*
>>> > pBuffer,ULONG32 ulSize)
>>> > {
>>> >     HXBOOL failed = FALSE;
<--------------------
>>> >
>>> >     Bitstream bs;
>>> >
>>> >     bs.SetBuffer(pBuffer);
>>> >     bs.SetBufSize(ulSize);
>>> >     if (m_bConfigPresent)
>>> >     {
>>> >         if (bs.GetBits(1) == 0)
>>> >         {
>>> >             // Not sure what we are
supposed to do if this fails
>>> >             if (!HandleMuxConfig(bs))
>>> >                 failed = TRUE;
>>> >         }
>>> >     }
>>> >
>>> >     if (!failed)
>>> >     {
>>> >         for (ULONG32 i = 0; i <
m_muxConfig.NumSubFrames(); i++)
>>> >         {
>>> >             ULONG32 ulFrameTime =
m_ulCurrentTime;
>>> >
>>> >             ulFrameTime += (i *
m_ulFrameDuration);
>>> >
>>> >  -          GetPayloadLengths(bs);
>>> > +           if (!GetPayloadLengths(bs))
>>> > +           {
>>> > +                return !failed ;
>>> > +           }
>>> >         }
>>> >
>>> >         m_bHadLoss = FALSE;
>>> >         m_ulBytesStaged = 0;
>>> >         m_bNeedTime = TRUE;
>>> >     }
>>> >
>>> >     return !failed;
>>> > }
>>> >
>>> > At 06:11 PM 6/26/2006, Eric Hyche wrote:
>>> >
>>> >
>>> >
>>> >       > -        
GetPayloadLengths(bs);
>>> >       > +         if
(!GetPayloadLengths(bs))
>>> >       > +            {
>>> >       > +                return !failed
;
>>> >       > +            }
>>> >       >           GetPayloads(bs,
ulFrameTime);
>>> >
>>> >       What is "failed" - I
can't tell from the diff.
>>> >
>>> >
>>> > "failed" initialized to FALSE.
(see definition of
>>> > ::ProcessStagingBuffer() above).
>>> >
>>> >
>>> >
>>> >       Just from the diff, it looks like
the sense
>>> >       of this return may be wrong - if
GetPayloadLengths()
>>> >       returns FALSE, which do you want to
do: return
>>> >       success or failure?
>>> >
>>> >
>>> > Here if GetPayloadLengths() return FALSE 
(in case of error)
>>> > ,  ::ProcessStagingBuffer() to return
failure as
>>> >
>>> > ::ProcessStagingBuffer() to return TRUE  
i.e. (!failed)
>>> >
>>> >  +           if (!GetPayloadLengths(bs))
>>> >  +            {
>>> >  +                return !failed ;
>>> >  +            }
>>> >
>>> > Thanks and regards,
>>> > ~Anil Jaiswal
>>> >
>>> >
>>> >
>>> >
>>> >       Eric
>>> >
>>> >       >
>>> >       > Modified by: ajaiswalreal.com
>>> >       > Date: 06-22-2006
>>> >       > Project: datatype
>>> >       > Branch:HEAD,
hxclient_1_5_0_cayenne
>>> >       >
>>> >       > Synopsis:
MP4LATMDepack::GetPayloadLengths -
>>> > m_pSlotLengths computation is
>>> >       > not safe
>>> >       >
>>> >       > Overview:
>>> >       > The computed m_pSlotLengths
size checked against the
>>> > Bitstream size to
>>> >       > prevent GetPayloads
>>> >       > method to read out of BitStream
bounds, as BitStream
>>> > class does not check
>>> >       > if GetBits method
>>> >       > reads outside the stream.
>>> >       >
>>> >       > Files Modified:
>>> >       >
datatype/mp4/payload/mp4-latm-depack.cpp
>>> >       >
datatype/mp4/payload/pub/mp4-latm-depack.h
>>> >       >
>>> >       > Image Size and Heap Use impact:
>>> >       > Minimal
>>> >       >
>>> >       > Platforms and Profiles
Affected:
>>> >       > all
>>> >       >
>>> >       > Distribution Libraries
Affected:
>>> >       > none
>>> >       >
>>> >       > Distribution library impact and
planned action:
>>> >       > n/a
>>> >       >
>>> >       > Platforms and Profiles Build
Verified:
>>> >       > win32-i386-vc7,
helix-client-all-defines
>>> >       >
>>> >       > Platforms and Profiles
Functionality verified:
>>> >       > win32-i386-vc7,
helix-client-all-defines
>>> >       >
>>> >       > Branch:
>>> >       > HEAD, hxclient_1_5_0_cayenne
>>> >       >
>>> >       > Diffs:
>>> >       > Index: mp4-latm-depack.cpp
>>> >       >
>>> >
============================================================
=======
>>> >       > RCS file:
/cvsroot/datatype/mp4/payload/mp4-latm-depack.cpp,v
>>> >       > retrieving revision 1.5.30.2
>>> >       > diff -u -w -r1.5.30.2
mp4-latm-depack.cpp
>>> >       > --- mp4-latm-depack.cpp       
4 Jan 2006 06:55:11
>>> > -0000       1.5.30.2
>>> >       > +++ mp4-latm-depack.cpp       
22 Jun 2006 18:05:43 -0000
>>> >       >  -242,7 +242,6 
>>> >       >
>>> >       >       bs.SetBuffer(pBuffer);
>>> >       >       bs.SetBufSize(ulSize);
>>> >       > -
>>> >       >       if (m_bConfigPresent)
>>> >       >       {
>>> >       >       if (bs.GetBits(1) == 0)
>>> >       >  -261,7 +260,10 
>>> >       >
>>> >       >           ulFrameTime += (i *
m_ulFrameDuration);
>>> >       >
>>> >       > -        
GetPayloadLengths(bs);
>>> >       > +         if
(!GetPayloadLengths(bs))
>>> >       > +            {
>>> >       > +                return !failed
;
>>> >       > +            }
>>> >       >           GetPayloads(bs,
ulFrameTime);
>>> >       >       }
>>> >       >
>>> >       >  -286,7 +288,7 
>>> >       >       return ret;
>>> >       >   }
>>> >       >
>>> >       > -void
MP4LATMDepack::GetPayloadLengths(Bitstream& bs)
>>> >       > +HXBOOL
MP4LATMDepack::GetPayloadLengths(Bitstream& bs)
>>> >       >   {
>>> >       >       if
(m_muxConfig.AllSameTiming())
>>> >       >       {
>>> >       >  -308,6 +310,11 
>>> >       >                         
m_pSlotLengths[streamID] += tmp;
>>> >       >                     } while
(tmp == 0xff);
>>> >       >
>>> >       > +                    if
(m_pSlotLengths[streamID] >
>>> > bs.GetBufSize())
>>> >       > +                    {
>>> >       > +                        return
FALSE;
>>> >       > +                    }
>>> >       > +
>>> >       >                     // Resize
the payload buffer if
>>> > we encounter a payload
>>> >       >                     // size
that is larger than our buffer
>>> >       >                     if
(m_pSlotLengths[streamID] >
>>> > m_ulPayloadBufSize)
>>> >       >  -321,6 +328,7 
>>> >       >           }
>>> >       >       }
>>> >       >       }
>>> >       > +    return TRUE;
>>> >       >   }
>>> >       >
>>> >       >   void
MP4LATMDepack::GetPayloads(Bitstream& bs,
>>> > ULONG32 ulTime)
>>> >       > Index: pub/mp4-latm-depack.h
>>> >       >
>>> >
============================================================
=======
>>> >       > RCS file:
>>> >
/cvsroot/datatype/mp4/payload/pub/mp4-latm-depack.h,v
>>> >       > retrieving revision 1.1.12.2
>>> >       > diff -u -w -r1.1.12.2
mp4-latm-depack.h
>>> >       > --- pub/mp4-latm-depack.h     4
Jan 2006 07:49:14
>>> > -0000       1.1.12.2
>>> >       > +++ pub/mp4-latm-depack.h    
22 Jun 2006 18:05:44 -0000
>>> >       >  -77,7 +77,7 
>>> >       >       HXBOOL
ProcessStagingBuffer(const UINT8*
>>> pBuffer,ULONG32
>>> >       > ulMuxConfigSize);
>>> >       >
>>> >       >       HXBOOL
HandleMuxConfig(Bitstream& bs);
>>> >       > -    void
GetPayloadLengths(Bitstream& bs);
>>> >       > +    HXBOOL
GetPayloadLengths(Bitstream& bs);
>>> >       >       void
GetPayloads(Bitstream& bs, ULONG32 ulTime);
>>> >       >
>>> >       >   private:
>>> >       >
>>> >       >
>>> >       >
>>> >       >
_______________________________________________
>>> >       > Datatype-dev mailing list
>>> >       > Datatype-devhelixcommunity.org
>>> >       >
>>> > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
>>> > <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>
>>> >       >
>>> >
>>> >
>>> >       ===================================
>>> >       Eric Hyche (ehychereal.com)
>>> >       Technical Lead
>>> >       RealNeworks, Inc.
>>> >
>>> >
>>> >
>
>
>
>

=======================================
Eric Hyche (ehychereal.com)
Technical Lead
RealNetworks, Inc.

_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

CR-Client Bug 4914MP4LATMDepack::GetPayloadLengths -m_pSlotLengths computation is not saf

Thread: CR-Client Bug 4914MP4LATMDepack::GetPayloadLengths -m_pSlotLengths computation is not saf