Hi Sheldon,

Thank you for your comments, based on the concerns I have modified the fix proposal, can you please review the same:
@@ -335,12 +335,13 @@
 {
 	int PossibleID_Length = 0;
 	binary PossibleIdNSize[16];
 	int PossibleSizeLength;
 	uint64 SizeUnknown;
 	int ReadIndex = 0; // trick for the algo, start index at 0
+	int ClusterReadIndex = 0; // counter for read octets
 	uint32 ReadSize = 0;
 	uint64 SizeFound;
 	int SizeIdx;
 	bool bFound;
 	int UpperLevel_original = UpperLevel;
 	
@@ -365,12 +367,19 @@
 			}
 
 			if (ReadIndex >= 4) {
 				// ID not found
 				// shift left the read octets
 				memmove(&PossibleIdNSize[0],&PossibleIdNSize[1], --ReadIndex);
+				ClusterReadIndex++;
+			}
 
+                 // we have already searched for a cluster ID in the array
+			if (ClusterReadIndex >= 16)
+			{
+				return NULL;
 			}
 
 			if (DataStream.read(&PossibleIdNSize[ReadIndex++], 1) == 0) {
 				return NULL; // no more data ?
 			}
 			ReadSize++;

Here we will be searching for cluster (4 bytes at a time) for the array, each time dropping the least significant byte if a valid cluster is not found in those 4 bytes. Usually we should get a cluster if present in 4 bytes, but still checking till whole array has been searched.

Best Regards,
Debashis. 

-----Original Message-----
From: ext Sheldon Fu [mailto:sfu at real.com] 
Sent: 08 February, 2011 19:04
To: Panigrahi Debashis.2 (EXT-Sasken/Bangalore)
Cc: datatype-dev at helixcommunity.org
Subject: Re: [datatype-dev] URGENT CR: ou1cimx1#461871: "Photos /Camera- While opening a corrupt MMC from photos/ camera device hangs

Right, you'll read until ReadIndex is 16 but since the newly-read byte 
is put at [ReadIndex] location in the read buffer and you only shift 3 
bytes each time to the left (SizeInx is always smaller than 4). This 
means that those bytes read into [>3] will never be shifted to position 
0 thus to be used in the loop at all, isn't it? E.g. when ReadIndex is 
4, The DataStream.read puts the new byte at location [4] then increase 
ReadIndex. Next round, with your change, you'll just shipt [1], [2], [3] 
three bytes left. The new byte readin at [4] will never be used at all. 
This doesn't seem to be right unless I mis-read something.

fxd


ext-debashis.2.panigrahi at nokia.com wrote:
> Hi Sheldon,
>
> We are always doing a byte read after these statements but after incrementing the ReadIndex, although we can stop reading after ReadIndex is 4.
> We will be coming to these lines (memmove () etc) only when cluster is not found in the current octet, in which case both ReadIndex and SizeIdx is 4, where we are shifting the current octet. Hence after this change we will be reading till ReadIndex is 16, within which a cluster should be present.
>
> Also the MaxDataSize that is sent is always 0xFFFFFFFF here, so theoretically it's not possible for ReadSize to exceed that value, and if a valid cluster is not found, it goes into an infinite loop. 
>
> Best Regards,
> Debashis.
>
> -----Original Message-----
> From: ext Sheldon Fu [mailto:sfu at real.com] 
> Sent: 08 February, 2011 16:14
> To: Panigrahi Debashis.2 (EXT-Sasken/Bangalore)
> Cc: datatype-dev at helixcommunity.org
> Subject: Re: [datatype-dev] URGENT CR: ou1cimx1#461871: "Photos /Camera- While opening a corrupt MMC from photos/ camera device hangs
>
> What's the reason for this change?
>
> -                               memmove(&PossibleIdNSize[0],&PossibleIdNSize[1], --ReadIndex);
> +                              memmove(&PossibleIdNSize[0],&PossibleIdNSize[1], --SizeIdx);
>
> It seems that this change will break the read  statement following it -- 
> you'll never use any byte read after ReadIndex > 4.
>
> And "it's impossible for a 32 bit integer to exceed a 64 bit one. " is 
> not correct. The length of 'max' variable might be 64-bit but that it 
> doesn't mean its actual value is beyond 32-bit. The change of ReadSize 
> is fine, but not for this reason.
>
> fxd
>
> ext-debashis.2.panigrahi at nokia.com wrote:
>   
>> Any comments on this?
>> ________________________________________
>> From: Panigrahi Debashis.2 (EXT-Sasken/Bangalore)
>> Sent: 04 February 2011 09:16
>> To: datatype-dev at helixcommunity.org
>> Subject: CR: ou1cimx1#461871: "Photos /Camera- While opening a corrupt MMC from photos/ camera device hangs
>>
>> "Nokia submits this code under the terms of a commercial contribution agreement with Real Networks, and I am authorized to contribute this code under said agreement."
>>
>> Modified by:  ext-debashis.2.panigrahi at nokia.com
>>
>> Reviewed by: girish.shetty at nokia.com
>>
>> RC Id: ou1cimx1#461871
>>
>> Date: 01/04/2011
>>
>> Project: SymbianMmf_wm
>>
>> Synopsis: Photos /Camera- While opening a corrupt MMC from photos/ camera device hangs
>>
>> Overview:
>> The content file is broken therefore file parser is not able to seek/ find next cluster after approximately 18 seconds of playback. Hence a hang is observed as the counter gets stuck in the loop for getting next element, which is un-available. This also leads to constant memory consumption and fragmentation, as constantly audio packets (in this case) are being created.
>>
>> Fix:
>> Exiting out of the function if 'ReadIndex' is more than 16, which means a valid cluster is not present. This fix will just prevent the hang and un-necessary memory consumption for this file, the file still cannot be played after 18secs. Also changing the type of 'ReadSize' to unsigned integer 64 bit for better comparison as 'MaxDataSize' is a 64 bit integer, and it's impossible for a 32 bit integer to exceed a 64 bit one.
>>
>> Files modified & changes:
>> xiph/matroskalib/libebml/src/EbmlElement.cpp
>>
>> Image Size and Heap Use impact: No major impact
>>
>> Module Release testing (STIF) : Passed
>>
>> Test case(s) Added  : No
>>
>> Memory leak check performed : Passed, No additional leaks introduced.
>>
>> Platforms and Profiles Build Verified: helix-client-s60-52-mmf-mdf-dsp
>>
>> Platforms and Profiles Functionality verified: armv5, winscw
>>
>> Branch: 210CayS, 420Brizo and HEAD
>>
>> CVS Diff on 210CayS:
>> Index: EbmlElement.cpp
>> ===================================================================
>> RCS file: /cvsroot/xiph/matroskalib/libebml/src/EbmlElement.cpp,v
>> retrieving revision 1.1.1.1.2.1
>> diff -u -w -r1.1.1.1.2.1 EbmlElement.cpp
>> --- EbmlElement.cpp     19 Mar 2010 19:25:34 -0000      1.1.1.1.2.1
>> +++ EbmlElement.cpp     2 Feb 2011 09:21:07 -0000
>> @@ -338,7 +338,7 @@
>>         int PossibleSizeLength;
>>         uint64 SizeUnknown;
>>         int ReadIndex = 0; // trick for the algo, start index at 0
>> -       uint32 ReadSize = 0;
>> +       uint64 ReadSize = 0;
>>         uint64 SizeFound;
>>         int SizeIdx;
>>         bool bFound;
>> @@ -348,6 +348,10 @@
>>                 // read a potential ID
>>                 do {
>>                         assert(ReadIndex < 16);
>> +                       if (ReadIndex >= 16)
>> +                       {
>> +                               return NULL;
>> +                       }
>>                         // build the ID with the current Read Buffer
>>                         bFound = false;
>>                         binary IdBitMask = 1 << 7;
>> @@ -367,7 +371,7 @@
>>                         if (ReadIndex >= 4) {
>>                                 // ID not found
>>                                 // shift left the read octets
>> -                               memmove(&PossibleIdNSize[0],&PossibleIdNSize[1], --ReadIndex);
>> +                              memmove(&PossibleIdNSize[0],&PossibleIdNSize[1], --SizeIdx);
>>                         }
>>
>>                         if (DataStream.read(&PossibleIdNSize[ReadIndex++], 1) == 0) {)
>>
>> _______________________________________________
>> Datatype-dev mailing list
>> Datatype-dev at helixcommunity.org
>> http://lists.helixcommunity.org/mailman/listinfo/datatype-dev
>> .
>>
>>   
>>     
>
> .
>
>