>> I assume that sizeof(uint32_t) comes from a few lines above: >> >> data = (uint32_t *)malloc(info_length); > > Note that this malloc cast is both ugly and unnecessary, this is not C++. > malloc() returns void*, which is compatible with any pointer type.
In an attempt to address both Diego and Rathann's comments, I have put together these 4 patches. Let me know what you think. Thanks E -- Erik Hovland [email protected] http://hovland.org/
From 67710d54393add9def1c3a71238ce917aadc8f34 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 call --- src/ifo_read.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) 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))) { free(vts_ptt_srpt); ifofile->vts_ptt_srpt = 0; return 0; -- 1.7.4.1
From 9c2d35e221f3faece3cfb5d6cf2f6745d5906172 Mon Sep 17 00:00:00 2001 From: Erik Hovland <[email protected]> Date: Sun, 10 Jul 2011 11:26:44 -0700 Subject: [PATCH 2/4] Clean up the error path of ifoRead_VTS. Rathann suggested that the error path in this function could be more cleanly called w/ a goto structure instead. --- src/ifo_read.c | 55 ++++++++++++++++++++++++------------------------------- 1 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/ifo_read.c b/src/ifo_read.c index fc5b39b..c0251c3 100644 --- a/src/ifo_read.c +++ b/src/ifo_read.c @@ -1143,29 +1143,23 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) { int info_length, i, j; uint32_t *data; - if(!ifofile) - return 0; - - if(!ifofile->vtsi_mat) - return 0; - - if(ifofile->vtsi_mat->vts_ptt_srpt == 0) /* mandatory */ - return 0; + /* mandatory sanity checks on the ifofile struct */ + if(!ifofile || !ifofile->vtsi_mat || ifofile->vtsi_mat->vts_ptt_srpt == 0) + goto ifoRead_VTS_err_return; if(!DVDFileSeek_(ifofile->file, ifofile->vtsi_mat->vts_ptt_srpt * DVD_BLOCK_LEN)) - return 0; + goto ifoRead_VTS_err_return; vts_ptt_srpt = (vts_ptt_srpt_t *)malloc(sizeof(vts_ptt_srpt_t)); if(!vts_ptt_srpt) - return 0; + goto ifoRead_VTS_err_return; ifofile->vts_ptt_srpt = vts_ptt_srpt; 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 ifoRead_VTS_free_vts_err_return; } B2N_16(vts_ptt_srpt->nr_of_srpts); @@ -1177,17 +1171,12 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) { info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE; - if(!(data = malloc(info_length))) { - free(vts_ptt_srpt); - ifofile->vts_ptt_srpt = 0; - return 0; - } + if(!(data = malloc(info_length))) + goto ifoRead_VTS_zero_ifo_vts_err_return; + 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 ifoRead_VTS_free_data_err_return; } for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) { @@ -1202,12 +1191,9 @@ 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) { - free(vts_ptt_srpt); - free(data); - ifofile->vts_ptt_srpt = 0; - return 0; - } + if(!vts_ptt_srpt->title) + goto ifoRead_VTS_free_data_err_return; + for(i = 0; i < vts_ptt_srpt->nr_of_srpts; i++) { int n; if(i < vts_ptt_srpt->nr_of_srpts - 1) @@ -1225,10 +1211,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 ifoRead_VTS_free_data_err_return; } 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 +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; } -- 1.7.4.1
From a32498a50c32b424488c862a928724bd74031065 Mon Sep 17 00:00:00 2001 From: Erik Hovland <[email protected]> Date: Sun, 10 Jul 2011 11:29:15 -0700 Subject: [PATCH 3/4] Conditional formatting changes --- 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 c0251c3..d2f7415 100644 --- a/src/ifo_read.c +++ b/src/ifo_read.c @@ -1149,7 +1149,9 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) { if(!DVDFileSeek_(ifofile->file, ifofile->vtsi_mat->vts_ptt_srpt * DVD_BLOCK_LEN)) + { goto ifoRead_VTS_err_return; + } vts_ptt_srpt = (vts_ptt_srpt_t *)malloc(sizeof(vts_ptt_srpt_t)); if(!vts_ptt_srpt) @@ -1200,10 +1202,12 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) { 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 c1f5f7472f33d91e4962499706af1e17272823b7 Mon Sep 17 00:00:00 2001 From: Erik Hovland <[email protected]> Date: Sun, 10 Jul 2011 11:31:38 -0700 Subject: [PATCH 4/4] Check the size of the PTT against what is referenced At least the region 1 DVD of _True_Grit_ can exhibit a situation where the PTT that is allocated and read is smaller then what gets referenced. The data is byte-swapped in place. This conditional will keep the read from going forward w/ this inconsistency in the data struct. Thanks goes to John Stebbins (stebbins AT jetheaddev DOT com) for the original patch. --- src/ifo_read.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/ifo_read.c b/src/ifo_read.c index d2f7415..cb26e8a 100644 --- a/src/ifo_read.c +++ b/src/ifo_read.c @@ -1181,6 +1181,11 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) { goto ifoRead_VTS_free_data_err_return; } + if(vts_ptt_srpt->nr_of_srpts * sizeof(uint32_t) > info_length) { + fprintf(stderr, "libdvdread: PTT search table too small.\n"); + goto ifoRead_VTS_free_data_err_return; + } + 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
