2.01.01a>=40: multi.c
Fix for multi-extent bug in multi.c has [at least] two bugs:
1. If directory entries constituting multi-extent file end up in
different sectors read_merging_directory ends up in endless loop.
Problematic loop is denoted in attached diff along with possible solution.
2. Given that mkisofs reserves for file system layout generated by
another iso9660 formatter, alternative partitioning of file to multiple
extents (for example larger amount of smaller extents) for now would
result in effective data corruption. Point is that it's not enough to
copy extents' isorec.extent fields alone, one has to copy at least
isorec.size fields as well. As for "at least." *Formally* (i.e. knowing
nothing about alternative iso9660 formatting and assuming "worst") one
should copy even isorec.ext_attr_length, [some bits from] flags,
file_unit_size and interleave. Attached diff denotes problematic code by
simply copying all of above mentioned fields in single memcpy (along
with isorec.date field, which is checked to be same as in curr_entry, so
copying should not cause side effects).
Couple of comments not related to above file-system formatting bugs.
There is inconsistency in directory_entry.mxpart declaration and its
usage. Declaration is "guarded" by #ifdef USE_LARGEFILES, while its
usage is not. Formally multi-extent files don't have to be "large," i.e.
"small" files are allowed to be multi-extent too (actually original
application for multi-extent was *CD* packet writing). Therefore it,
multi-extent support, should not be associated with USE_LARGEFILES and
mxpart declaration should not be depend on corresponding macro.
In check_prev_session one can see free(ptr[i]); and in copy_mult_ext one
can see free(sex);. These are pointers to directory_entry structure,
which in turn can contain pointers to other dynamically allocated
objects, most notably rr_attributes and name. Freeing an object without
freeing other dynamically allocated objects it alone refers to used to
constitute memory leaks.
As for copy_mult_extent. As mentioned above it copies pieces of
information from original extent records. As alternative to this field
copying would it be appropriate to consider simply using original extent
records, i.e. removing them from orig_contents array and linking them
into this_dir? A.
P.S. As usual I make no claims on correctness of provided patch, it
serves primarily educational purposes.
--- ./multi.c.orig 2008-06-13 22:40:10.000000000 +0200
+++ ./multi.c 2008-07-23 10:42:42.000000000 +0200
@@ -761,26 +761,11 @@
if ((mx & ISO_MULTIEXTENT) == 0 &&
(idr->flags[0] & ISO_MULTIEXTENT) != 0) {
struct directory_entry *s_entry;
- struct iso_directory_record *idr2 = idr;
- int i2 = i;
- off_t tsize = 0;
-
- /*
- * Sum up the total file size for the multi extent file
- */
- while (i2 < len) {
- idr2 = (struct iso_directory_record *) &dirbuff[i2];
-
- tsize += get_733(idr2->size);
- if ((idr2->flags[0] & ISO_MULTIEXTENT) == 0)
- break;
- i2 += idr2->length[0];
- }
s_entry = dup_directory_entry(*pnt); /* dup first for mxroot */
s_entry->de_flags |= MULTI_EXTENT;
s_entry->de_flags |= INHIBIT_ISO9660_ENTRY|INHIBIT_JOLIET_ENTRY;
- s_entry->size = tsize;
+ s_entry->size = 0; /* size is calculated below */
s_entry->starting_block = (*pnt)->starting_block;
s_entry->mxroot = s_entry;
s_entry->mxpart = 0;
@@ -796,6 +781,7 @@
(pnt[-1])->next = *pnt;
(*pnt)->mxroot = (pnt[-1])->mxroot;
(*pnt)->mxpart = (pnt[-1])->mxpart + 1;
+ (*pnt)->mxroot->size += (*pnt)->size; /* accumulate mxroot's size */
}
pnt++;
mx = idr->flags[0];
@@ -1184,7 +1170,16 @@
se1->next = sex;
len1++;
}
- memcpy(se1->isorec.extent, se2->isorec.extent, 8);
+ /* 27 bytes memcpy covers following fields:
+ ext_attr_length,
+ extent,
+ size,
+ date,
+ flags,
+ file_unit_size,
+ interleave
+ */
+ memcpy(se1->isorec.ext_attr_length,se2->isorec.ext_attr_length,27);
se1->starting_block = get_733(se2->isorec.extent);
se1->de_flags |= SAFE_TO_REUSE_TABLE_ENTRY;
se1->de_flags |= MULTI_EXTENT;
@@ -1195,7 +1190,7 @@
se1 = se1->next;
se2 = se2->next;
}
- memcpy(se1->isorec.extent, se2->isorec.extent, 8);
+ memcpy(se1->isorec.ext_attr_length,se2->isorec.ext_attr_length,27);
se1->starting_block = get_733(se2->isorec.extent);
se1->de_flags |= SAFE_TO_REUSE_TABLE_ENTRY;
se1->isorec.flags[0] &= ~ISO_MULTIEXTENT; /* Last entry */
Reply to: