Fix for multi-extent bug in multi.c has [at least] two bugs:
These two bugs were discovered by merely looking at the code. Actual initial testings revealed [at least] 3 more bugs, see below.
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.Coiuld you please explant your claim?
How are 2KB sectors in directory table filled with directory entries? If there is no place for next directory entry in current 2KB sector the latter gets padded with 0s and next directory entry is written to next 2KB sector. Now all you have to imagine is sequence of directory entries constituting same multi-extent file being laid down in two adjacent sectors with 0 padding between them. Having this image in mind, how would following loop handle this padding?
while (i2 < len) {
idr2 = (struct iso_directory_record *) &dirbuff[i2];
...
i2 += idr2->length[0];
}
It would spin endlessly, wouldn't it? Well, actual loop had an extra
termination criteria:
while (i2 < len) {
idr2 = (struct iso_directory_record *) &dirbuff[i2];
if ((idr2->flags[0]&ISO_MULTIEXTENT)==0) break;
i2 += idr2->length[0];
}
So that there are two possible scenarios:
1. idr2->flags[0] overlaps with 0 padding, therefore condition is met
and loop gets terminated *prematurely*. This results in calculated
s_entry->size being *less* than it should (run mkisofs -M under debugger
and see for yourself).
2. If padding is small enough idr2->flags[0] reads byte of *some* data from next sector and if this byte happens to have ISO_MULTIEXTENT bit set, then the loop spins endlessly.
Well, my initial report was admittedly incomplete, I failed to mention case #1, but #2 is as real. Attached image (produced with mkisofs 2.01.01a43) triggers this case. The image contains single 40GB large file with long name, so that this file's extents' directory entries span over two 2KB blocks.
The code you removed is needed and you did not provide a working replacement.
Yes, I did! What is the purpose of the loop in question? To calculate size field in "super" directory entry preceding actual extent entries, isn't it? In next "if" I added following line:
(*pnt)->mxroot->size += (*pnt)->size; which takes proper care of this very field without any extra loop.
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 toThis is a claim that would need a prove...
Given last experience I can't even imagine that it would be possible for me to provide "proof" you would accept. You just have to experience it yourself:
1. mkdir test
touch test/file
perl -e 'truncate("test/file",8*1024*1024)'
this gives you test directory with 8MB file;
2. modify 2.01.01a43 mkisofs/tree.c and redefine LARGE_EXTENT and
MAX_EXTENT to say 0x100000, 1MB, recompile;
3. mkisofs -R -iso-level 3 test > test.isothis gives you an image with 8 extents generated by "alternative iso9660 formatter."
4. burn in to CD;5. modify mkisofs/tree.c and redefine LARGE_EXTENT and MAX_EXTENT to say 0x200000, 2MB, recompile;
6. collect multi-session parameters from cdrecord and simply "refresh" directory structure:
mkisofs -C xx,yy -M xxx -R -iso-level 3 test > test2.iso7. burn test2.iso as second session, mount it and tell us what you see, run 'wc /cdrom/test/file' and tell us what is the result.
Now to further bugs.Bug #3. If previous session has multi-extent file[s] read_merging_directory effectively omits last directory entry[-ies] in corresponding subdirectory, which naturally results in file[s] disappearing from multi-session recordings. This is because of following. Beside pointer to directory_entry* array procedure in question returns number of entries in this array in *nentp. Now amount of allocated and filled entries is nent+nmult, while value returned in nentp is nent alone. This means that if for example there is multi-extent file in current directory followed by single-extent one, latter disappears from multi-session recording, because consumer of returned array would never process it. One should return nent+nmult, see reference to line #591 in attached diff.
Bug #4. check_prev_session cleans up only first extent on multi-extent file. As result remaining extents are attempted to be merged, which in turn results in failure to sort merged directory. This happens because loop index in clean-up is never incremented, see reference to line #1099 in attached diff.
Bug #5. Whenever there is an iso-name clash with previous session and mkisofs assigns xxxxxNNN name to multi-extent file in previous session to resolve the conflict, it fails to "rename" the actual extents, only "super" directory entry. This in turn results in failure to sort merged directory with "have the same ISO9660 name" error. This happens because sort_n_finish relies on "super" entry to precede actual extents in linked list, while merge_remaining_entries flips the order so that "super" entry's ->next point at next file, not 1st own extent. See reference to line #1353 in attached diff for order preserving list grow.
Attached diff is cumulative, i.e. relative to a43, not to my previous post. 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-24 18:38:10.000000000 +0200
@@ -591,6 +591,7 @@
* one multi-extent root entry per multi-extent file.
*/
rtn = (struct directory_entry **) e_malloc((nent+nmult) * sizeof (*rtn));
+ nent += nmult;
/*
* Finally, scan the directory one last time, and pick out the relevant
@@ -761,26 +762,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 +782,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];
@@ -806,6 +793,7 @@
* If we find an associated file, check if there is a file
* with same ISO name and link it to this entry
*/
+ /* XXX how does one treat multi-extent here? */
for (pnt = rtn, i = 0; i < nent; i++, pnt++) {
int j;
@@ -1099,6 +1087,7 @@
while (j < len && ptr[j] && ptr[j]->mxroot == ptr[i]) {
free(ptr[j]);
ptr[j] = NULL;
+ j++;
}
}
if (odpnt != NULL) {
@@ -1184,7 +1173,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 +1193,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 */
@@ -1353,7 +1351,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;
}
@@ -1394,9 +1392,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;
}
Attachment:
a.star.gz
Description: GNU Zip compressed data