mkisofs multi-extent: more bugs
As per http://lists.debian.org/cdwrite/2008/07/msg00078.html and
http://lists.debian.org/cdwrite/2008/07/msg00082.html 6 (six) a>=40
multi.c bugs were reported:
#1. end-less loop [or premature exit from it] in read_merging_directory;
#2. incorrect merge of extents from previous session [generated by
alternative iso9660 formatter];
#3. files or extents are omitted from multi-session recording due to
incorrect value returned by read_merging_directory;
#4. failure to sort merged directory because of insufficient clean-up in
check_prev_session;
#5. failure to sort merged directory if multi-extent files share same
iso9660 name;
#6. apparent memory leak in check_prev_session;
Of these 6 bugs two were fixed, #1 and #4, and one, #3, kind of fixed.
"Kind of" means that the problem as it was spelled in the problem report
was resolved, but the thing is that affected procedure uses the return
value itself and modification let it use wrong value. See below for
further details. Rest of the problems were left without attention.
For reference, here is how to reproduce #5:
1. touch 5G; perl -e 'truncate("5G",5*1024*1024*1024)'
2. ./mkisofs -iso-level=3 -R 5G > a.iso
3. mv 5G 5g;
4. ./mkisofs -C X,Z -M a.iso -iso-level=3 -R 5g > /dev/null;
which results in fatal failure:
Using 5G000.;1 for /5G (5g)
./mkisofs: Error: '/5G' and '5g' have the same ISO9660 name '5G.;1'.
./mkisofs: Error: '/5G' and '/5G' have the same Rock Ridge name '5G'.
./mkisofs: Unable to sort directory
Further bugs.
#7. './mkisofs -iso-level=3 -T 5g' is aborted with "*** glibc detected
*** double free or corruption." core file analysis reveals that fatal
condition is raised in free called from sort_n_finish (there is only one
free call in sort_n_finish). This is because dup_directory_entry does
not duplicate ->table member, and therefore the latter is bound to be
double-freed.
#8. Once #7 is resolved './mkisofs -iso-level=3 -T 5g' completes, but
TRANS.TBL comes out bloated with duplicating entries: one entry per
recorded extent instead of one per whole multi-extent file.
#9. Once #8 is resolved, multi-extent names and names of last file[s] in
current directory are found to be omitted from TRANS.TBL from
multi-session recording with -T flag. Multi-extent names are omitted
because only "super" directory entry is processed by
read_merging_directory, and as it's marked with INHIBIT_ISO9660_ENTRY
it's never "nominated" for TRANS.TBL. Single-extent files can be omitted
from TRANS.TBL because of skewed fix for bug #3 (see above).
Attached patch relative to a53 addresses remaining and new problems.
In tree.c:
- vicinity of lines 573 and 713 - #8;
- vicinity of line 2403 - #7;
In multi.c:
- vicinity of line 805 - #3 and #9;
- vicinity of line 918 - #9;
- vicinity of line 943 - #3;
- vicinity of lines 1090 and 1148 - alternative solution to #2 based on
copying of actual extents from previous session, not partial information
from them, as well as #6;
- vicinity of lines 1357 and 1398 - #5, code was submitted earlier and
its purpose is to maintain expected list order;
- vicinity of line 1869 - #6;
On related note to original report. tree.c also has inconsistencies
between usage of USE_LARGEFILES and multi-extent support. A.
--- ./mkisofs/tree.c.orig 2008-09-11 16:01:02.000000000 +0200
+++ ./mkisofs/tree.c 2008-12-11 10:56:10.000000000 +0100
@@ -573,6 +573,12 @@
#endif /* APPLE_HYB */
if (s_entry->de_flags & INHIBIT_ISO9660_ENTRY)
continue;
+ /*
+ * In every ISO_MULTIEXTENT chain last entry
+ * is the only one without the flag.
+ */
+ if (s_entry->isorec.flags[0] & ISO_MULTIEXTENT)
+ continue;
if (s_entry->table) {
/*
* Max namelen, a space before and a space
@@ -713,6 +719,12 @@
if (s_entry->de_flags & INHIBIT_ISO9660_ENTRY)
continue;
/*
+ * In every ISO_MULTIEXTENT chain last entry
+ * is the only one without the flag.
+ */
+ if (s_entry->isorec.flags[0] & ISO_MULTIEXTENT)
+ continue;
+ /*
* Warning: we cannot use the return value of sprintf
* because old BSD based sprintf() implementations
* will return a pointer to the result instead of a
@@ -2403,6 +2415,8 @@
s_entry1->name = e_strdup(s_entry->name);
if (s_entry->whole_name)
s_entry1->whole_name = e_strdup(s_entry->whole_name);
+ if (s_entry->table)
+ s_entry1->table = e_strdup(s_entry->table);
#ifdef APPLE_HYB
/*
* If we also duplicate s_entry->hfs_ent, we would need to change
--- ./mkisofs/multi.c.orig 2008-08-28 23:06:34.000000000 +0200
+++ ./mkisofs/multi.c 2008-12-11 11:02:06.000000000 +0100
@@ -76,7 +76,7 @@
LOCAL void free_directory_entry __PR((struct directory_entry * dirp));
LOCAL int iso_dir_ents __PR((struct directory_entry *de));
LOCAL void copy_mult_extent __PR((struct directory_entry *se1,
- struct directory_entry *se2));
+ struct directory_entry *se2[]));
LOCAL void merge_remaining_entries __PR((struct directory *,
struct directory_entry **, int));
@@ -805,6 +805,8 @@
mx = idr->flags[0];
i += idr->length[0];
}
+
+ nent += nmult; /* traverse *all* entries */
#ifdef APPLE_HYB
/*
* If we find an associated file, check if there is a file
@@ -819,6 +821,16 @@
if (strncmp(rtn[j]->isorec.name,
(*pnt)->isorec.name, rlen) == 0 &&
(rtn[j]->isorec.flags[0] & ISO_ASSOCIATED) == 0) {
+
+ if (rtn[j]->isorec.flags[0] & ISO_MULTIEXTENT) {
+ comerrno(EX_BAD,
+ "Implementation botch. "
+ "ISO_ASSOCIATED and ISO_MULTIEXTENT "
+ "don't go together for %.*s\n",
+ rlen,(*pnt)->isorec.name);
+
+ }
+
rtn[j]->assoc = *pnt;
/*
@@ -918,7 +930,14 @@
}
(*pnt)->name = e_strdup(p);
}
- break;
+ /*
+ * Process all extents up to
+ * last in multi-extent chain.
+ * Otherwise multi-extent files
+ * are omitted from TRANS.TBL.
+ */
+ if (!((*pnt)->isorec.flags[0] & ISO_MULTIEXTENT))
+ break;
}
}
cpnt = cpnt1 + 1;
@@ -943,7 +962,7 @@
if (dirbuff != NULL) {
free(dirbuff);
}
- *nentp = nent + nmult;
+ *nentp = nent;
return (rtn);
} /* read_merging_directory */
@@ -1090,25 +1109,21 @@
if ((curr_entry->isorec.flags[0] & ISO_MULTIEXTENT) ||
(ptr[i]->isorec.flags[0] & ISO_MULTIEXTENT)) {
- copy_mult_extent(curr_entry, ptr[i]);
+ copy_mult_extent(curr_entry, &ptr[i]);
}
goto found_it;
}
return (retcode);
found_it:
- if (ptr[i]->mxroot == ptr[i]) { /* Remove all multi ext. entries */
- int j = i + 1; /* First one will be removed below */
-
- while (j < len && ptr[j] && ptr[j]->mxroot == ptr[i]) {
- free(ptr[j]);
- ptr[j++] = NULL;
- }
- }
+ /*
+ * if it was multi-extent file, copy_mult_extent called above
+ * wiped ptr[i+1...] and moved the extents to curr_entry list.
+ */
if (odpnt != NULL) {
*odpnt = ptr[i];
} else {
- free(ptr[i]);
+ free_directory_entry(ptr[i]);
}
ptr[i] = NULL;
return (retcode);
@@ -1148,69 +1163,67 @@
LOCAL void
copy_mult_extent(se1, se2)
struct directory_entry *se1;
- struct directory_entry *se2;
+ struct directory_entry *se2[];
{
- struct directory_entry *curr_entry = se1;
- int len1;
- int len2;
- int mxpart = 0;
-
- len1 = iso_dir_ents(se1);
- len2 = iso_dir_ents(se2);
+ struct directory_entry *next_file,*extent;
+ int i;
- if (len1 == 1) {
+ if (se1->mxroot == NULL) {
/*
* Convert single-extent to multi-extent.
* If *se1 is not multi-extent, *se2 definitely is
* and we need to set up a MULTI_EXTENT directory header.
*/
- se1->de_flags |= MULTI_EXTENT;
+ se1->de_flags |= MULTI_EXTENT|INHIBIT_ISO9660_ENTRY|INHIBIT_JOLIET_ENTRY;
se1->isorec.flags[0] |= ISO_MULTIEXTENT;
- se1->mxroot = curr_entry;
+ se1->mxroot = se1;
se1->mxpart = 0;
- se1 = dup_directory_entry(se1);
- curr_entry->de_flags |= INHIBIT_ISO9660_ENTRY|INHIBIT_JOLIET_ENTRY;
- se1->de_flags |= INHIBIT_UDF_ENTRY;
- se1->next = curr_entry->next;
- curr_entry->next = se1;
- se1 = curr_entry;
- len1 = 2;
- }
-
- while (se2->isorec.flags[0] & ISO_MULTIEXTENT) {
- len1--;
- len2--;
- if (len1 <= 0) {
- struct directory_entry *sex = dup_directory_entry(se1);
-
- sex->mxroot = curr_entry;
- sex->next = se1->next;
- se1->next = sex;
- len1++;
- }
- memcpy(se1->isorec.extent, se2->isorec.extent, 8);
- se1->starting_block = get_733(se2->isorec.extent);
- se1->de_flags |= SAFE_TO_REUSE_TABLE_ENTRY;
- se1->de_flags |= MULTI_EXTENT;
- se1->isorec.flags[0] |= ISO_MULTIEXTENT;
- se1->mxroot = curr_entry;
- se1->mxpart = mxpart++;
+ next_file = se1->next;
+ }
+ else {
+ /*
+ * Drop all extents...
+ */
+ next_file = se1->next;
+ while (next_file && next_file->mxroot == se1) {
+ extent = next_file;
+ next_file = extent->next;
+ free_directory_entry(extent);
+ }
+ }
- se1 = se1->next;
- se2 = se2->next;
+ /*
+ * ... and copy extents from previous session.
+ */
+ extent = se1;
+ extent->next = se2[0]->next;
+ /*
+ * We don't know how large is se2[], but its elements are even
+ * linked into a list. extent->next!=NULL detects the end of
+ * the list and thus the end of se2[]...
+ */
+ for (i=1; extent->next!=NULL && se2[i]->mxroot==se2[0]; ++i) {
+ extent = se2[i];
+ extent->mxroot = se1;
+ extent->de_flags |= SAFE_TO_REUSE_TABLE_ENTRY;
+ se2[i] = NULL;
}
- memcpy(se1->isorec.extent, se2->isorec.extent, 8);
- se1->starting_block = get_733(se2->isorec.extent);
- se1->de_flags |= SAFE_TO_REUSE_TABLE_ENTRY;
- se1->isorec.flags[0] &= ~ISO_MULTIEXTENT; /* Last entry */
- se1->mxpart = mxpart;
- while (len1 > 1) { /* Drop other entries */
- struct directory_entry *sex;
-
- sex = se1->next;
- se1->next = sex->next;
- free(sex);
- len1--;
+
+ /*
+ * Link remaining chain[s].
+ */
+ se2[0]->next = extent->next; /* for aesthetic reasons */
+ extent->next = next_file;
+
+ if (i==1) {
+ /*
+ * File was single-extent in previous session,
+ * convert multi-extent to single-extent.
+ */
+ se1->de_flags &= ~(MULTI_EXTENT|INHIBIT_ISO9660_ENTRY|INHIBIT_JOLIET_ENTRY);
+ se1->isorec.flags[0] &= ~ISO_MULTIEXTENT;
+ se1->mxroot = NULL;
+ se1->mxpart = 0;
}
}
@@ -1357,7 +1370,7 @@
* Whatever is leftover in the list needs to get merged back into the
* directory.
*/
- for (i = 0; i < n_orig; i++) {
+ for (s_entry = NULL, i = 0; i < n_orig; i++) {
if (pnt[i] == NULL) {
continue;
}
@@ -1398,9 +1411,15 @@
merge_old_directory_into_tree(pnt[i], this_dir);
}
}
- pnt[i]->next = this_dir->contents;
- pnt[i]->filedir = this_dir;
- this_dir->contents = pnt[i];
+ if (s_entry == NULL) {
+ pnt[i]->next = this_dir->contents;
+ this_dir->contents = pnt[i];
+ }
+ else {
+ pnt[i]->next = s_entry->next;
+ s_entry->next = pnt[i];
+ }
+ s_entry = pnt[i];
pnt[i] = NULL;
}
@@ -1869,7 +1888,7 @@
}
}
if (odpnt) {
- free(odpnt);
+ free_directory_entry(odpnt);
odpnt = NULL;
}
/*
Reply to: