[DVDnav-discuss] [PATCH] libdvdread: Fix crash when PTT is too short

[John Stebbins]

On 07/10/2011 03:09 PM, Erik Hovland wrote:
>>> diff --git a/src/ifo_read.c b/src/ifo_read.c
>>> index 4a422c6..fc5b39b 100644
>>> --- a/src/ifo_read.c
>>> +++ b/src/ifo_read.c
>>> @@ -1177,8 +1177,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
>>>
>>>     info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE;
>>>
>>> -  data = (uint32_t *)malloc(info_length);
>>> -  if(!data) {
>>> +  if(!(data = malloc(info_length))) {
>> Personally I prefer not to merge the lines, I think it makes the code
>> harder to read, but that's not important..
> Fine w/ me. I fixed the other malloc to not have the cast as well.
>
>> Probably not this way though.
>> If you initialize data and vts_ptt_srpt to NULL
>> you only need
>>> err_out:
>>>    free(data);
>>>    ifofile->vts_ptt_srpt = 0;
>>>    free(vts_ptt_srpt);
>>>    return 0;
> New patch does it the way you suggest.
>> The ifoRead_VTS is only makes the name unwieldy, gotos can't go across
>> functions.
> The label was long because I kept forgetting which function I was in.
> This version removes it.
>
>>> +  if(vts_ptt_srpt->nr_of_srpts * sizeof(uint32_t)>  info_length) {
>> The suggestion was to use vts_ptt_srpt->nr_of_srpts * sizeof(*data)
>> so that the check matches the reads the code does no matter what type
>> data has.
>> And while not an issue here for several reasons I'd recommend going with
>> vts_ptt_srpt->nr_of_srpts>  info_length / sizeof(*data)
>> which is safe from integer overflows.
>> Considering how often getting this wrong leads to exploitable bugs IMO
>> it's a good idea to always use the paranoid version in the hopes that
>> everyone gets used to doing it in the safe way.
> Let's take the high road!
>
> Thanks for the review.
>
> E
>

The discussion on this kind of petered out.  Is this going to be committed?  Are there additional changes that need to 
be made?  The only additional comment I see is about vts_ptt_srpt->title cleanup which was already a loose end before 
the proposed patch and should be solved in it's own patch.