[MPlayer-dev-eng] [PATCH]libdvdread: seqfault and patch to fix it (again)

[Frédéric Marchal]

Hello,

On July 11th, Morten Sjøgren reported a segfault due to the
double free of a buffer in ifoFree_PTL_MAIT and provided
a patch:

http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2010-July/066025.html

The patch was taken into account by Dominik 'Rathann' Mierzejewski
on September 2nd:

http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2010-September/066033.html

The patch is incomplete as ifoRead_PTL_MAIT can still return without
resetting the pointer in ifofile->ptl_mait. In my case, I have two
DVDs failing due an invalid seek offset passed to DVDFileSeek_().

The following patch set ifofile->ptl_mait to NULL (not 0 as in the
original patch) before every return statement.

Moreover, if the seek offset is obviously out of range, the loop over
the countries is interrupted and the nr_of_countries is set to the
index of the highest country that could be read. I'm not sure it is
the proper action but without a test case to disprove it, it sound
sensible as nr_of_countries doesn't seem to be used.

Frederic

---
Index: libdvdread4/ifo_read.c
===================================================================
--- libdvdread4/ifo_read.c	(révision 1218)
+++ libdvdread4/ifo_read.c	(copie de travail)
@@ -281,7 +281,8 @@ static void read_playback_type(playback_type_t *pt
 static void free_ptl_mait(ptl_mait_t* ptl_mait, int num_entries) {
   int i;
   for (i = 0; i < num_entries; i++)
-    free(ptl_mait->countries[i].pf_ptl_mai);
+    if (ptl_mait->countries[i].pf_ptl_mai)
+      free(ptl_mait->countries[i].pf_ptl_mai);
 
   free(ptl_mait->countries);
   free(ptl_mait);
@@ -1303,7 +1304,7 @@ int ifoRead_PTL_MAIT(ifo_handle_t *ifofile) {
 
   if(!(DVDReadBytes(ifofile->file, ptl_mait, PTL_MAIT_SIZE))) {
     free(ptl_mait);
-    ifofile->ptl_mait = 0;
+    ifofile->ptl_mait = NULL;
     return 0;
   }
 
@@ -1322,16 +1323,19 @@ int ifoRead_PTL_MAIT(ifo_handle_t *ifofile) {
   ptl_mait->countries = (ptl_mait_country_t *)malloc(info_length);
   if(!ptl_mait->countries) {
     free(ptl_mait);
-    ifofile->ptl_mait = 0;
+    ifofile->ptl_mait = NULL;
     return 0;
   }
+  for(i = 0; i < ptl_mait->nr_of_countries; i++) {
+    ptl_mait->countries[i].pf_ptl_mai = NULL;
+  }
 
   for(i = 0; i < ptl_mait->nr_of_countries; i++) {
     if(!(DVDReadBytes(ifofile->file, &ptl_mait->countries[i], PTL_MAIT_COUNTRY_SIZE))) {
       fprintf(stderr, "libdvdread: Unable to read PTL_MAIT.\n");
       free(ptl_mait->countries);
       free(ptl_mait);
-      ifofile->ptl_mait = 0;
+      ifofile->ptl_mait = NULL;
       return 0;
     }
   }
@@ -1351,24 +1355,39 @@ int ifoRead_PTL_MAIT(ifo_handle_t *ifofile) {
   for(i = 0; i < ptl_mait->nr_of_countries; i++) {
     uint16_t *pf_temp;
 
+    if (ptl_mait->countries[i].pf_ptl_mai_start_byte
+                + 8*2 * (ptl_mait->nr_of_vtss + 1) > ptl_mait->last_byte + 1) {
+       fprintf(stderr, "libdvdread: should have %d countries but pointer to country %d is invalid.
\n",
+               ptl_mait->nr_of_countries, i);
+       /*
+       It may not be a good idea to truncate the number of countries at the first
+       invalid pointer. A test case is necessary to determine a more suitable
+       action.
+       */
+       ptl_mait->nr_of_countries=i;
+       break;
+    }
     if(!DVDFileSeek_(ifofile->file,
                      ifofile->vmgi_mat->ptl_mait * DVD_BLOCK_LEN
                      + ptl_mait->countries[i].pf_ptl_mai_start_byte)) {
-      fprintf(stderr, "libdvdread: Unable to seek PTL_MAIT table.\n");
+      fprintf(stderr, "libdvdread: Unable to seek PTL_MAIT table at index %d.\n",i);
       free(ptl_mait->countries);
       free(ptl_mait);
+      ifofile->ptl_mait = NULL;
       return 0;
     }
     info_length = (ptl_mait->nr_of_vtss + 1) * sizeof(pf_level_t);
     pf_temp = (uint16_t *)malloc(info_length);
     if(!pf_temp) {
       free_ptl_mait(ptl_mait, i);
+      ifofile->ptl_mait = NULL;
       return 0;
     }
     if(!(DVDReadBytes(ifofile->file, pf_temp, info_length))) {
-      fprintf(stderr, "libdvdread: Unable to read PTL_MAIT table.\n");
+      fprintf(stderr, "libdvdread: Unable to read PTL_MAIT table at index %d.\n",i);
       free(pf_temp);
       free_ptl_mait(ptl_mait, i);
+      ifofile->ptl_mait = NULL;
       return 0;
     }
     for (j = 0; j < ((ptl_mait->nr_of_vtss + 1) * 8); j++) {
@@ -1378,6 +1397,7 @@ int ifoRead_PTL_MAIT(ifo_handle_t *ifofile) {
     if(!ptl_mait->countries[i].pf_ptl_mai) {
       free(pf_temp);
       free_ptl_mait(ptl_mait, i);
+      ifofile->ptl_mait = NULL;
       return 0;
     }
     { /* Transpose the array so we can use C indexing. */
@@ -1402,7 +1422,8 @@ void ifoFree_PTL_MAIT(ifo_handle_t *ifofile) {
 
   if(ifofile->ptl_mait) {
     for(i = 0; i < ifofile->ptl_mait->nr_of_countries; i++) {
-      free(ifofile->ptl_mait->countries[i].pf_ptl_mai);
+      if (ifofile->ptl_mait->countries[i].pf_ptl_mai)
+        free(ifofile->ptl_mait->countries[i].pf_ptl_mai);
     }
     free(ifofile->ptl_mait->countries);
     free(ifofile->ptl_mait);