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

[Erik Hovland]

>> 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

-- 
Erik Hovland
erik at hovland.org
http://hovland.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Clean-up-malloc-calls.patch
Type: text/x-patch
Size: 1434 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20110710/96e67f11/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Clean-up-error-paths.patch
Type: text/x-patch
Size: 2623 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20110710/96e67f11/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Conditional-style-change.patch
Type: text/x-patch
Size: 1501 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20110710/96e67f11/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Fix-crash-when-PTT-is-too-short.patch
Type: text/x-patch
Size: 1094 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20110710/96e67f11/attachment-0007.bin>