[DVDnav-discuss] Libdvdread misses hidden files and causes segfaults to calling programs

[Reimar Döffinger]

On 20 Sep 2011, at 00:53, Erik Hovland <erik at hovland.org> wrote:

>>>> This patch has been submitted twice already, but I like my version
>>>> better.  The other 2 unnecessarily truncate the
>>>> string when something is found in the high byte.  This fills that string
>>>> the same as pre-patch, but returns a code
>>>> indicating that junk was found in the MSB.
>>> 
>>> I[n] that case I don't see the point in making things hard to review
>>> by rewriting the whole function.
>>> Why not just
>>> Index: dvd_udf.c
>>> ===================================================================
>>> --- dvd_udf.c   (revision 1233)
>>> +++ dvd_udf.c   (working copy)
>>> @@ -329,16 +329,17 @@
>>>  static int Unicodedecode( uint8_t *data, int len, char *target )
>>>  {
>>>    int p = 1, i = 0;
>>> +  int err = 0;
>>> 
>>>    if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do {
>>> -    if( data[ 0 ] == 16 ) p++;  /* Ignore MSB of unicode16 */
>>> +    if( data[ 0 ] == 16 ) err |= data[ p++ ];  /* Ignore MSB of unicode16
>>> */
>>>      if( p<  len ) {
>>>        target[ i++ ] = data[ p++ ];
>>>      }
>>>    } while( p<  len );
>>> 
>>>    target[ i ] = '\0';
>>> -  return 0;
>>> +  return !err;
>>>  }
>>> 
>>>  static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
>> 
>> In that case, you need to observe the return value in UDFDescriptor, which
>> isn't done currenty:
>> 
>>  static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
>> @@ -490,7 +501,9 @@
>>   L_FI = GETN1(19);
>>   UDFLongAD(&data[20], FileICB);
>>   L_IU = GETN2(36);
>> -  if (L_FI) Unicodedecode(&data[38 + L_IU], L_FI, FileName);
>> +  if (L_FI) {
>> +    if (!Unicodedecode(&data[38 + L_IU], L_FI, FileName)) FileName[0] =
>> '\0';
>> +  }
>>   else FileName[0] = '\0';
>>   return 4 * ((38 + L_FI + L_IU + 3) / 4);
>>  }
> 
> I don't think Reimar was actually submitting a patch but rather making
> a critique of the Unicodedecode() part of the patch.

Exactly, it is also completely untested and not all that carefully verified.
And I don't mind a rewrite of the function, it is not very nice as it is currently.
However I do not like combining rewrite and bug fixes without a need.
And also (no offence intended) the rewritten functions didn't look too great to me.
Until we are done with bike-shedding on a rewritten function it might be some time.
So my suggestion is a "we are really sure we didn't break anything due to a typo or such" kind of change first.
>