[Libcdio-devel] [RFC] Hid ISO_MAX_MULTIEXTENT from public and made extent list dynamic memory

[Thomas Schmitt]

Hi,

first changeset in new branch ts-multiextent :

"Hid ISO_MAX_MULTIEXTENT from public and made extent list dynamic memory"
  
http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-multiextent&id=3b602d69cb116a297ee95e48471a097ce7456435


This is still Pete Batard's (compatible) API extension which would break ABI
by changing existing member offsets in public struct iso9660_stat_s.

As the commit message says, this change solves the problem of a publicly
known array size which would cause ABI breaking each time it gets enlarged.
It also reduces memory waste with single-extent files and gave me the
opportunity to push the file size limit to ~ 400 GiB without extra cost.

I refrained for now from changing the two arrays .extent_lsn and .extent_size
to a single array of a two-member struct, because this keeps valid the
changes by Pete in examples/ and test/.
But i had to transmogrify Pete's hack about "[# extents]" in example/isolist.c

---

Tested after ./configure --disable-shared  by 

  $ valgrind example/isolist .../multi_extent_8k.iso
  ...
  ==27675== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
  ...
  Preparer   : XORRISO-1.4.9 2018.05.18.152119, LIBISOBURN-1.4.9, 
LIBISOFS-1.4.9, LIBBURN-1.4.9
  Volume     : ISOIMAGE
  d [LSN       50]         2048 /.
  d [LSN       50]         2048 /..
  - [LSN       55]        54305 /multi_extent_file [7 extents]
  ...
  ==27675== All heap blocks were freed -- no leaks are possible
  ...
  ==27675== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 1 from 1)

  $ valgrind example/isolist .../file_of_4gb.iso
  ...
  Preparer   : XORRISO-1.2.2 2012.04.02.133001, LIBISOBURN-1.2.2, 
LIBISOFS-1.2.2, LIBBURN-1.2.2
  Volume     : ISOIMAGE
  d [LSN       50]         2048 /.
  d [LSN       50]         2048 /..
  - [LSN  2098231]            4 /aaa
  - [LSN       55]   4297064448 /file_of_4gb [2 extents]
  - [LSN  2098232]            4 /zzz
  ...
  ==28079== All heap blocks were freed -- no leaks are possible
  ...
  ==28079== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 1 from 1)

(Tempus fugit.)

==========================================================================

diff --git a/example/isolist.c b/example/isolist.c
index 2a6c4b3..61c385b 100644
--- a/example/isolist.c
+++ b/example/isolist.c
@@ -70,7 +70,7 @@ main(int argc, const char *argv[])
   char const *psz_fname;
   iso9660_t *p_iso;
   const char *psz_path = "/";
-  char psz_extents[] = " [# extents]";
+  char psz_extents[] = " [4294967296 extents]"; /* enough room for uint32_t */
 
   if (argc > 1)
     psz_fname = argv[1];
@@ -109,7 +109,8 @@ main(int argc, const char *argv[])
       iso9660_name_translate(p_statbuf->filename, filename);
 
        if (p_statbuf->num_extents > 1)
-           psz_extents[2] = '0' + p_statbuf->num_extents;
+           sprintf(psz_extents, " [%lu extents]",
+                   (unsigned long) p_statbuf->num_extents);
        printf ("%s [LSN %8d] %12" PRIi64 " %s%s%s\n",
                _STAT_DIR == p_statbuf->type ? "d" : "-",
                p_statbuf->extent_lsn[0], p_statbuf->total_size, psz_path, 
filename,
diff --git a/include/cdio/iso9660.h b/include/cdio/iso9660.h
index 819834b..01fd8f2 100644
--- a/include/cdio/iso9660.h
+++ b/include/cdio/iso9660.h
@@ -156,9 +156,6 @@ extern enum iso_vd_enum_s {
 /*! \brief Maximum number of characters in a volume-set id. */
 #define ISO_MAX_VOLUMESET_ID 128
 
-/*! \brief Maximum number of multi file extent licdio supports. */
-#define ISO_MAX_MULTIEXTENT 8
-
 /*! String inside frame which identifies an ISO 9660 filesystem. This
     string is the "id" field of an iso9660_pvd_t or an iso9660_svd_t.
 */
@@ -550,9 +547,9 @@ struct iso9660_stat_s { /* big endian!! */
                      /**v combined size of all extents, in bytes */
   uint64_t           total_size;
                      /**v start logical sector number for each extent */
-  lsn_t              extent_lsn[ISO_MAX_MULTIEXTENT];
+  lsn_t              *extent_lsn;
                      /**v size of each extent */
-  uint32_t           extent_size[ISO_MAX_MULTIEXTENT];
+  uint32_t           *extent_size;
   /* NB: If you need to access the 'secsize' equivalent for an extent,
    * you should use CDIO_EXTENT_BLOCKS(extent_size[extent_nr]) */
 
diff --git a/lib/iso9660/iso9660_fs.c b/lib/iso9660/iso9660_fs.c
index 2d48fab..3f351f0 100644
--- a/lib/iso9660/iso9660_fs.c
+++ b/lib/iso9660/iso9660_fs.c
@@ -99,6 +99,10 @@ static long int iso9660_seek_read_framesize (const iso9660_t 
*p_iso,
                                             long int size,
                                             uint16_t i_framesize);
 
+/*! \brief Maximum number of multi file extent licdio supports. */
+#define ISO_MAX_MULTIEXTENT 100
+
+
 /* Adjust the p_iso's i_datastart, i_byte_offset and i_framesize
    based on whether we find a frame header or not.
 */
@@ -775,11 +779,38 @@ iso9660_check_dir_block_end(iso9660_dir_t *p_iso9660_dir, 
unsigned *offset)
 }
 
 
+static bool
+_iso9660_statbuf_alloc_extents(iso9660_stat_t *p_stat, int num_extents)
+{
+  p_stat->extent_lsn = calloc(num_extents, sizeof(lsn_t));
+  if (p_stat->extent_lsn == NULL) {
+no_memory:;
+    cdio_warn("Could not allocate %lu bytes",
+             (unsigned long) num_extents * (sizeof(lsn_t) + sizeof(uint32_t)));
+    return false;
+  }
+  p_stat->extent_size = calloc(num_extents, sizeof(uint32_t));
+  if (p_stat->extent_size == NULL) {
+    free(p_stat->extent_lsn);
+    goto no_memory;
+  }
+  return true;
+}
+
 
+/*!
+   @param single_extent Submit true to allocate only a single extent.
+                        Submit false to allocate the maximum number of extents.
+                        In the latter case, call _iso9660_statbuf_adj_extents()
+                        when all extents are assessed.
+                        Rule of thumb: If last_p_stat is submitted as constant
+                        NULL, then single_extent should be true, else false.
+*/
 static iso9660_stat_t *
 _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir,
                         iso9660_stat_t *last_p_stat,
-                        bool_3way_t b_xa, uint8_t u_joliet_level)
+                        bool_3way_t b_xa, uint8_t u_joliet_level,
+                        bool single_extent)
 {
   uint8_t dir_len= iso9660_get_dir_len(p_iso9660_dir);
   iso711_t i_fname;
@@ -802,6 +833,14 @@ _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir,
     cdio_warn("Couldn't calloc(1, %d)", stat_len);
     return NULL;
   }
+  if (last_p_stat == NULL) {
+    if (!_iso9660_statbuf_alloc_extents(p_stat, single_extent ?
+                                                1 : ISO_MAX_MULTIEXTENT)) {
+      free(p_stat);
+      return NULL;
+    }
+  }
+
   p_stat->type    = (p_iso9660_dir->file_flags & ISO_DIRECTORY)
     ? _STAT_DIR : _STAT_FILE;
   p_stat->extent_lsn[p_stat->num_extents] = from_733 (p_iso9660_dir->extent);
@@ -938,6 +977,28 @@ _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir,
 
 }
 
+
+static bool
+_iso9660_statbuf_adj_extents(iso9660_stat_t *p_stat)
+{
+  lsn_t *lsn;
+  uint32_t *sizes;
+
+  lsn = p_stat->extent_lsn;
+  sizes =  p_stat->extent_size;
+  if (!_iso9660_statbuf_alloc_extents(p_stat, p_stat->num_extents)) {
+    p_stat->extent_lsn = lsn;
+    p_stat->extent_size = sizes;
+    return false;
+  }
+  memcpy(p_stat->extent_lsn, lsn, p_stat->num_extents * sizeof(lsn_t));
+  memcpy(p_stat->extent_size, sizes, p_stat->num_extents * sizeof(uint32_t));
+  free(lsn);
+  free(sizes);
+  return true;
+}
+
+
 /*!
   Return the directory name stored in the iso9660_dir_t
 
@@ -1009,7 +1070,7 @@ _fs_stat_root (CdIo_t *p_cdio)
 #endif
 
     p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, NULL,
-                                     b_xa, p_env->u_joliet_level);
+                                     b_xa, p_env->u_joliet_level, true);
     return p_stat;
   }
 
@@ -1031,7 +1092,8 @@ _ifs_stat_root (iso9660_t *p_iso)
 
   p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, NULL,
                                    p_iso->b_xa,
-                                   p_iso->u_joliet_level);
+                                   p_iso->u_joliet_level,
+                                   true);
   return p_stat;
 }
 
@@ -1085,7 +1147,7 @@ _fs_stat_traverse (const CdIo_t *p_cdio, const 
iso9660_stat_t *_root,
        continue;
 
       p_iso9660_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, NULL,
-                                       dunno, p_env->u_joliet_level);
+                                       dunno, p_env->u_joliet_level, true);
 
       cmp = strcmp(splitpath[0], p_iso9660_stat->filename);
 
@@ -1181,7 +1243,8 @@ _fs_iso_stat_traverse (iso9660_t *p_iso, const 
iso9660_stat_t *_root,
        continue;
 
       p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, p_stat,
-                                       p_iso->b_xa, p_iso->u_joliet_level);
+                                       p_iso->b_xa, p_iso->u_joliet_level,
+                                       false);
 
       if (!p_stat) {
        cdio_warn("Bad directory information for %s", splitpath[0]);
@@ -1193,6 +1256,11 @@ _fs_iso_stat_traverse (iso9660_t *p_iso, const 
iso9660_stat_t *_root,
       if (p_iso9660_dir->file_flags & ISO_MULTIEXTENT)
         continue;
 
+      if (!_iso9660_statbuf_adj_extents(p_stat)) {
+       free(_dirbuf);
+       return NULL;
+      }
+
       cmp = strcmp(splitpath[0], p_stat->filename);
 
       if ( 0 != cmp && 0 == p_iso->u_joliet_level
@@ -1439,10 +1507,16 @@ iso9660_fs_readdir (CdIo_t *p_cdio, const char 
psz_path[])
 
        p_iso9660_stat = _iso9660_dir_to_statbuf(p_iso9660_dir,
                                                 p_iso9660_stat, dunno,
-                                                p_env->u_joliet_level);
+                                                p_env->u_joliet_level,
+                                                false);
        if ((p_iso9660_stat) &&
            ((p_iso9660_dir->file_flags & ISO_MULTIEXTENT) == 0))
          {
+            if (!_iso9660_statbuf_adj_extents(p_stat)) {
+              iso9660_stat_free(p_stat);
+              iso9660_dirlist_free(retval);
+              return NULL;
+            }
            _cdio_list_append (retval, p_iso9660_stat);
            p_iso9660_stat = NULL;
          }
@@ -1524,10 +1598,17 @@ iso9660_ifs_readdir (iso9660_t *p_iso, const char 
psz_path[])
        p_iso9660_stat = _iso9660_dir_to_statbuf(p_iso9660_dir,
                                                 p_iso9660_stat,
                                                 p_iso->b_xa,
-                                                p_iso->u_joliet_level);
+                                                p_iso->u_joliet_level,
+                                                false);
        if ((p_iso9660_stat) &&
            ((p_iso9660_dir->file_flags & ISO_MULTIEXTENT) == 0))
          {
+           if (!_iso9660_statbuf_adj_extents(p_stat)) {
+             _cdio_list_free (retval, true, NULL);
+             iso9660_stat_free(p_stat);
+             free (_dirbuf);
+             return NULL;
+           }
            _cdio_list_append(retval, p_iso9660_stat);
            p_iso9660_stat = NULL;
          }
@@ -1739,6 +1820,8 @@ iso9660_stat_free(iso9660_stat_t *p_stat)
     if (p_stat->rr.psz_symlink) {
       CDIO_FREE_IF_NOT_NULL(p_stat->rr.psz_symlink);
     }
+    CDIO_FREE_IF_NOT_NULL(p_stat->extent_lsn);
+    CDIO_FREE_IF_NOT_NULL(p_stat->extent_size);
     free(p_stat);
   }
 }
@@ -1811,7 +1894,7 @@ iso_have_rr_traverse (iso9660_t *p_iso, const 
iso9660_stat_t *_root,
         continue;
 
       p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, NULL, p_iso->b_xa,
-                                       p_iso->u_joliet_level);
+                                       p_iso->u_joliet_level, true);
       have_rr = p_stat->rr.b3_rock;
       if ( have_rr != yep) {
         if (strlen(splitpath[0]) == 0)

==========================================================================

Have a nice day :)

Thomas