>> 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
[email protected]
http://hovland.org/
From d5aa6a762c856ec07e264491479e35531763279e Mon Sep 17 00:00:00 2001
From: Erik Hovland <[email protected]>
Date: Sun, 10 Jul 2011 11:12:27 -0700
Subject: [PATCH 1/4] Clean up malloc calls
Both malloc calls have a cast to the type of the pointer's
type. This is not necessary in C and is poor style.
---
src/ifo_read.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/ifo_read.c b/src/ifo_read.c
index 4a422c6..8e3b1b7 100644
--- a/src/ifo_read.c
+++ b/src/ifo_read.c
@@ -1141,7 +1141,7 @@ void ifoFree_TT_SRPT(ifo_handle_t *ifofile) {
int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
vts_ptt_srpt_t *vts_ptt_srpt;
int info_length, i, j;
- uint32_t *data;
+ uint32_t *data = NULL;
if(!ifofile)
return 0;
@@ -1156,7 +1156,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
ifofile->vtsi_mat->vts_ptt_srpt * DVD_BLOCK_LEN))
return 0;
- vts_ptt_srpt = (vts_ptt_srpt_t *)malloc(sizeof(vts_ptt_srpt_t));
+ vts_ptt_srpt = malloc(sizeof(vts_ptt_srpt_t));
if(!vts_ptt_srpt)
return 0;
@@ -1176,8 +1176,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
CHECK_VALUE(vts_ptt_srpt->nr_of_srpts < 100); /* ?? */
info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE;
-
- data = (uint32_t *)malloc(info_length);
+ data = malloc(info_length);
if(!data) {
free(vts_ptt_srpt);
ifofile->vts_ptt_srpt = 0;
--
1.7.4.1
From 59b0398576d6de77c3ea69af10d59499f168829c Mon Sep 17 00:00:00 2001
From: Erik Hovland <[email protected]>
Date: Sun, 10 Jul 2011 14:55:48 -0700
Subject: [PATCH 2/4] Clean up error paths.
Clean up the error paths of the function using goto.
---
src/ifo_read.c | 31 +++++++++++++------------------
1 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/ifo_read.c b/src/ifo_read.c
index 8e3b1b7..68cf171 100644
--- a/src/ifo_read.c
+++ b/src/ifo_read.c
@@ -1139,7 +1139,7 @@ void ifoFree_TT_SRPT(ifo_handle_t *ifofile) {
int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
- vts_ptt_srpt_t *vts_ptt_srpt;
+ vts_ptt_srpt_t *vts_ptt_srpt = NULL;
int info_length, i, j;
uint32_t *data = NULL;
@@ -1164,8 +1164,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
if(!(DVDReadBytes(ifofile->file, vts_ptt_srpt, VTS_PTT_SRPT_SIZE))) {
fprintf(stderr, "libdvdread: Unable to read PTT search table.\n");
- free(vts_ptt_srpt);
- return 0;
+ goto fail;
}
B2N_16(vts_ptt_srpt->nr_of_srpts);
@@ -1178,16 +1177,11 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE;
data = malloc(info_length);
if(!data) {
- free(vts_ptt_srpt);
- ifofile->vts_ptt_srpt = 0;
- return 0;
+ goto fail;
}
if(!(DVDReadBytes(ifofile->file, data, info_length))) {
fprintf(stderr, "libdvdread: Unable to read PTT search table.\n");
- free(vts_ptt_srpt);
- free(data);
- ifofile->vts_ptt_srpt = 0;
- return 0;
+ goto fail;
}
for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) {
@@ -1203,10 +1197,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
vts_ptt_srpt->title = malloc(vts_ptt_srpt->nr_of_srpts * sizeof(ttu_t));
if(!vts_ptt_srpt->title) {
- free(vts_ptt_srpt);
- free(data);
- ifofile->vts_ptt_srpt = 0;
- return 0;
+ goto fail;
}
for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) {
int n;
@@ -1225,10 +1216,8 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
if(!vts_ptt_srpt->title[i].ptt) {
for(n = 0; n < i; n++)
free(vts_ptt_srpt->title[n].ptt);
- free(vts_ptt_srpt);
- free(data);
- ifofile->vts_ptt_srpt = 0;
- return 0;
+
+ goto fail;
}
for(j = 0; j < vts_ptt_srpt->title[i].nr_of_ptts; j++) {
/* The assert placed here because of Magic Knight Rayearth Daybreak */
@@ -1258,6 +1247,12 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
}
return 1;
+
+fail:
+ free(data);
+ ifofile->vts_ptt_srpt = 0;
+ free(vts_ptt_srpt);
+ return 0;
}
--
1.7.4.1
From 3549b05cd5b8b39f512d38ec3edcc34dfb0974d0 Mon Sep 17 00:00:00 2001
From: Erik Hovland <[email protected]>
Date: Sun, 10 Jul 2011 14:59:38 -0700
Subject: [PATCH 3/4] Conditional style change
---
src/ifo_read.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/ifo_read.c b/src/ifo_read.c
index 68cf171..5f7ffa9 100644
--- a/src/ifo_read.c
+++ b/src/ifo_read.c
@@ -1176,9 +1176,9 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE;
data = malloc(info_length);
- if(!data) {
+ if(!data)
goto fail;
- }
+
if(!(DVDReadBytes(ifofile->file, data, info_length))) {
fprintf(stderr, "libdvdread: Unable to read PTT search table.\n");
goto fail;
@@ -1196,19 +1196,21 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
vts_ptt_srpt->ttu_offset = data;
vts_ptt_srpt->title = malloc(vts_ptt_srpt->nr_of_srpts * sizeof(ttu_t));
- if(!vts_ptt_srpt->title) {
+ if(!vts_ptt_srpt->title)
goto fail;
- }
+
for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) {
int n;
if(i < vts_ptt_srpt->nr_of_srpts - 1)
n = (data[i+1] - data[i]);
else
n = (vts_ptt_srpt->last_byte + 1 - data[i]);
+
/* assert(n > 0 && (n % 4) == 0);
Magic Knight Rayearth Daybreak is mastered very strange and has
Titles with 0 PTTs. */
if(n < 0) n = 0;
+
CHECK_VALUE(n % 4 == 0);
vts_ptt_srpt->title[i].nr_of_ptts = n / 4;
--
1.7.4.1
From cf815ce6a4daae2d5b04c1fa16af362938bc1f60 Mon Sep 17 00:00:00 2001
From: Erik Hovland <[email protected]>
Date: Sun, 10 Jul 2011 15:03:59 -0700
Subject: [PATCH 4/4] Fix crash when PTT is too short
The PTT that is allocated and read is smaller than what gets referenced.
The data is byte-swapped in place which results in writes to memory
locations outside the allocated region. Region 1 True Grit is an
example of this.
Derived from a patch submitted by John Stebbins. Thanks!
---
src/ifo_read.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/ifo_read.c b/src/ifo_read.c
index 5f7ffa9..15bade5 100644
--- a/src/ifo_read.c
+++ b/src/ifo_read.c
@@ -1184,6 +1184,10 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
goto fail;
}
+ if(vts_ptt_srpt->nr_of_srpts > info_length / sizeof(*data)) {
+ fprintf(stderr, "libdvdread: PTT search table too small.\n");
+ goto fail;
+ }
for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) {
B2N_32(data[i]);
/* assert(data[i] + sizeof(ptt_info_t) <= vts_ptt_srpt->last_byte + 1);
--
1.7.4.1
_______________________________________________
DVDnav-discuss mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
