'[datatype-dev] RE: CR/CN : Fix for bug 9761: it costs a long time'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       helix-datatype-dev
Subject:    [datatype-dev] RE: CR/CN : Fix for bug 9761: it costs a long time
From:       Umakant Gundeli <ugundeli () real ! com>
Date:       2009-12-23 5:32:35
Message-ID: 766B5A29D28DA442AB229AAEE2AFC44507D79E632C () SEAMBX ! corp ! real ! com
[Download RAW message or body]

Thanks a lot, Sheldon.
I incorporated your suggested m_ulTotalFileSize=0 change and checked in the fix only \
to hxclient_3_6_1_atlas branch for now.

thanks,
-Umakant.
________________________________________
From: Sheldon Fu [sfu@real.com]
Sent: Tuesday, December 22, 2009 8:04 PM
To: Umakant Gundeli
Cc: datatype-dev@helixcommunity.org; helixdev@real.com
Subject: Re: [datatype-dev] CR : Fix for bug 9761: it costs a long time when scanning \
an RA file.

Look ok to me. One minor problem, m_ulTotalFileSize is UNIT32 so you
shouldn't do m_ulTotalFileSize(-1) and compare it to -1. Should use zero
instead.

Can you check this in only for the release branch build and wait for
somebody familiar with rm fileformat and parsing to respond before you
check this into Head? I am a bit concerned what impact this change could
have on the server side (real server and client share the same rmff plugin).

fxd

Umakant Gundeli wrote:
> Thanks for review, Sheldon.
> 
> I completely reworked the fix and now I am using IHXFileStatResponse interface to \
> get all the file related information (for getting file size) and attached the \
> updated DIFF 
> Please see my inline comments.
> ________________________________________
> From: Sheldon Fu [sfu@real.com]
> Sent: Tuesday, December 22, 2009 4:39 PM
> To: Umakant Gundeli
> Cc: datatype-dev@helixcommunity.org; helixdev@real.com
> Subject: Re: [datatype-dev] CR : Fix for bug 9761: it costs a long time when \
> scanning an RA file. 
> 1. you can not use fopen/fseek/ftell to get file size. Riff reader uses
> a file object to read the data and you'll have to stick with that --
> getting size from file object.
> 
> --> updated logic does not use fopen/fseek/ftell.
> 
> 2. I don't quite understanding your description of the problem. If it is
> only the chunk id that is messed up, the scanning of chunks should still
> work, only that now it will see an unknown chunk (id=0x00000000) and
> should skip it correctly since you said the size field is intact.
> 
> --> I am sorry for previously missing on some important information.
> One more major problem with this corrupted RA file is the last chunk. the last \
> chunk's 4 byte chunk size is GREATER than it's actual body size. and that is why we \
> need  to bound the seek range by file size. 
> 3. In general though, it is good to bound the seek range by the file
> size (if file size is known that is) so the change could be a good thing
> though I am not sure why it fixes the problem you described.
> 
> Honestly speaking, even I am not sure why and how this change fixes this bug, but \
> it does. :) due to lack of time for more investigation (we need to get this fix for \
> wednesday's release), I am going with this fix. after this release, I will spend \
> more time to figure out how exactly this fixes the bug. 
> thanks,
> -Umakant.
> 
> fxd
> 
> Umakant Gundeli wrote:
> 
> > Project: Real Player for Android
> > 
> > Synopsis: Fix for bug 9761: it costs a long time when scanning an RA file.
> > 
> > Overview:
> > 
> > This bug was due to scanning of a corrupted RA file. In this given corrupted RA \
> > file, The 4 byte "CONT" chunk ID was missing. The CONT chunk ID \
> > (RM_CONTENT_OBJECT) value should be "0x434F4E54" BUT it was "0x00000000". This 4 \
> > byte chunk id (corrupted) was followed by a VALID 4 byte chunk size and then \
> > chunk body of valid size which had normal meta data values. So, the \
> > RM_CONTENT_OBJECT existed BUT only the chunk id CONT was messed up. 
> > When the RM  file format does m_pReader->FindChunk(RM_CONTENT_OBJECT, FALSE); the \
> > RIFF Reader traverses the whole file chunk by chunk to find it, but at one point \
> > the RIFF reader tries to SEEK to an OFFSET which is GREATER THAN the TOTAL FILE \
> > SIZE. after this SEEK command issue, it takes about 20 seconds to reach \
> > ::SeekDone() method. 
> > The RIFF Reader has the following code to check if it has reached the END of RM \
> > file, 
> > /datatype/common/util/riff.cpp
> > around line number 413
> > 
> > // Didn't find it, go to the next chunk.
> > m_state = RS_ChunkBodySeekPending;
> > m_ulSeekOffset = m_ulCurOffset + GetLong(&buf[4]);
> > 
> > /* Are we at the end of .rm file */
> > if  (m_ulSeekOffset == m_ulCurOffset &&
> > 
> > m_ulFileType != RIFF_FILE_MAGIC_NUMBER &&
> > m_ulFileType != IFF_FILE_MAGIC_NUMBER )
> > {
> > m_pResponse->RIFFFindChunkDone(HXR_FAILED, 0);
> > return HXR_OK;
> > }
> > 
> > In case of this type of corrupted RA file, this check will not work. As ONLY the \
> > chunk id is messed up as 0x00000000 ( leaving other parts of chunk untouched), \
> > the RIFF Reader may not be able to SKIP the chunk correctly. RIFF reader may read \
> > incorrect 4 byte sequences as chunk ID and chunk size; leading to wrong \
> > calculation of all further m_ulSeekOffset calculations. and thus if  \
> > (m_ulSeekOffset == m_ulCurOffset) may not work and it would continue trying to \
> > seek beyond the file limits. 
> > The correct way of RM EOF checking should be like this.
> > 
> > /* Are we at the end of .rm file */
> > if ( (m_ulSeekOffset == m_ulCurOffset || m_ulSeekOffset >= m_ulTotalFileSize ) &&
> > m_ulFileType != RIFF_FILE_MAGIC_NUMBER &&
> > m_ulFileType != IFF_FILE_MAGIC_NUMBER )
> > {
> > m_pResponse->RIFFFindChunkDone(HXR_FAILED, 0);
> > return HXR_OK;
> > }
> > 
> > Therefore, The solution is to calculate the total file size and use it to check \
> > and make sure RIFF reader is not trying to SEEK beyond file limits. 
> > 
> > Files Added: None
> > 
> > Files Modified:
> > datatype/common/util/riff.cpp
> > datatype/common/util/pub/riff.h
> > datatype-restricted/rm/fileformat/rmffplin.cpp
> > datatype-restricted/rm/fileformat/rmffplin.h
> > 
> > Platforms and Profiles Build and Functionality Verified:
> > Platform: android-donut-arm.eabi
> > Profile: helix-client-android
> > target(s): android_all
> > 
> > Branch: hxclient_3_6_1_atlas_restricted
> > hxclient_3_1_0_atlas
> > 
> > Attchment :
> > bug9761-DIFF.txt
> > 
> > thanks,
> > -Umakant.
> > ------------------------------------------------------------------------
> > 
> > _______________________________________________
> > Datatype-dev mailing list
> > Datatype-dev@helixcommunity.org
> > http://lists.helixcommunity.org/mailman/listinfo/datatype-dev
> > 
> 
> 

_______________________________________________
Datatype-dev mailing list
Datatype-dev@helixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/datatype-dev

[prev in list] [next in list] [prev in thread] [next in thread] 