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

[Reimar Döffinger]

On Sun, Jul 10, 2011 at 11:38:04AM -0700, 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..

> Rathann suggested that the error path in this function
> could be more cleanly called w/ a goto structure instead.

> @@ -1258,6 +1242,15 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
>    }
>  
>    return 1;
> +
> +ifoRead_VTS_free_data_err_return:
> +  free(data);
> +ifoRead_VTS_zero_ifo_vts_err_return:
> +  ifofile->vts_ptt_srpt = 0;
> +ifoRead_VTS_free_vts_err_return:
> +  free(vts_ptt_srpt);
> +ifoRead_VTS_err_return:
> +  return 0;

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;

The ifoRead_VTS is only makes the name unwieldy, gotos can't go across
functions.

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