Re: UDF filesystem fixes for grub

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]

On 11/02/2010 02:10 PM, Giuseppe Caizzone wrote:
> Hello,
>
> I've made 4 small patches that improve the UDF filesystem support in
> grub. With these changes, I've been able to read any regular file in
> UDF filesystems created by both Linux and Windows on both optical
> storage and USB sticks. Perhaps they might be useful for other grub
> users?
> I don't know if this list is the most appropriate place to post them,
> I apologise for the noise if it isn't.
>
> I'm attaching the patches, which are against last week's trunk, and a
> simple bash test case for each.
>
> - - - -
>
> 1/4) Support for large (> 2GiB) files
>
> I know that grub isn't a typical consumer of huge files, but since the
> fix is apparently trivial...
>
> How to test that the patch is needed (tried with grub trunk on linux 2.6.36):
>
> # dd if=/dev/zero of=/test.bin bs=1M count=3072
> # mkfs.udf /test.bin
> # mount -o loop /test.bin /mnt/m1
> # dd if=/dev/urandom of=/mnt/m1/bigfile.bin bs=1M count=2560
> # umount /mnt/m1
> # grub-fstest /test.bin crc "(loop0)/bigfile.bin"
>
> Result: grub will segfault instead of reading the whole file (tested
> on a 32-bit machine).
> The patch just changes an int and a grub_uint32_t to be grub_off_t.
>
>   
Good patch. Can you write the ChangeLog entry for it?
> 2/4) Support for deleted files
>
> On real-world, non-readonly media, deleted files are likely to appear
> inside directories. These files' directory entries have a "deleted"
> bit, which grub currently ignores.
>
> Test case:
>
> # dd if=/dev/zero of=/test.bin bs=1M count=32
> # mkfs.udf /test.bin
> # mount -o loop /test.bin /mnt/m1
> # touch /mnt/m1/file{1,2,3,4,5}
> # rm /mnt/m1/file3
> # umount /mnt/m1
> # grub-fstest /test.bin ls "(loop0)/"
>
> Result: only the first two files will be listed, because grub will
> "crash" when, encountering the third file's deleted directory entry,
> it tries to "dereference" a zeroed field.
> The patch just skips directory entries with the "deleted" bit set, in
> grub_udf_iterate_dir().
>
>   
This one is good too. Changelog entry?
> 3/4) Support for a generic block size
>
> Currently grub assumes that all UDF filesystems have a logical block
> size of 2048 bytes. But the UDF standard states that the block size
> should match the physical sector size of the hosting device, so for
> instance Windows only creates filesystems with a block size of 512
> bytes on USB sticks. Under Linux, mkudffs allows for other block sizes
> as well, up to 4096 bytes. None of these are currently readable by
> grub.
>
> Test case:
>
> # dd if=/dev/zero of=/test.bin bs=1M count=32
> # mkfs.udf --blocksize=512 /test.bin
> # grub-fstest /test.bin ls "(loop0)/"
>
> Result: the file system won't be recognised as UDF by grub.
> The patch repeats the superblock search in grub_udf_mount() for 512,
> 1024, 2048 and 4096 block sizes, stopping if it finds a valid one AND
> the sector offset recorded in that superblock matches the detected
> superblock offset. So for instance it won't misdetect a superblock
> located at sector 256 with blocksize 2048 for a superblock located at
> sector 512 with blocksize 1024.
> The log2(blocksize/512) is then stored in a new field called "lbshift"
> in struct grub_udf_data, which gets used instead of the previous
> constant GRUB_UDF_LOG2_BLKSZ, while the constant GRUB_UDF_BLKSZ gets
> replaced with the lvd.bsize field already present in struct
> grub_udf_data.
>
>   
+  lbshift = 0;
+  block = 0;
+  while (lbshift <= 3)
+    {
For loop is way more appropriate than a while loop
+  while (lbshift <= 3)
+    {
+      sblklist = sblocklist;
+      while (*sblklist)
+        {
Same here.
ChangeLog entry?
> 4/4) Support for allocation descriptor extensions
>
> When files on an UDF file system grow and fragmentation appears, an
> "allocation descriptor extension" entry can be recorded, which points
> to a new block containing more allocation descriptors for the file.
> Currently grub ignores these entries, and is thus unable to read such
> fragmented files properly.
>
> Test:
>
> # dd if=/dev/zero of=/test.bin bs=1M count=32
> # mkfs.udf --blocksize=512 /test.bin
> # mount -o loop /test.bin /mnt/m1
> # for ((i=0;i<8192;i++)) ; do touch "/mnt/m1/file$i" ; done
> # umount /mnt/m1
> # grub-fstest /test.bin ls "(loop0)/"
>
> Result: only the first 4820 files appear (because the directory itself
> gets fragmented, and when grub reaches the jump between the two
> fragments, it erroneously tries to read the allocation extension
> descriptor as the directory's body).
> The patch checks, in grub_udf_read_block(), whether the "allocation
> descriptor type", previously ignored, is 3, and in this case, it loads
> the referenced block, checks that it's a valid allocation extension
> descriptor, and sets the "ad" pointer (and the "len" limit) to
> continue the iteration from the allocation descriptors contained in
> that block.
>
> This last one is the only patch which actually contains a proper new
> block of code, and it allocates a big structure on the stack (it's a
> sector, so it's up to 4K big): I don't know if this is OK, or if
> perhaps I should use grub_malloc() instead. Also, the patch depends on
> the previous "generic block size" patch.
>
>   
+  char buf[U32 (node->data->lvd.bsize)];
Will segfault if bsize is too big. You need to allocate on heap
-  else
+  else if (U16 (node->fe.tag.tag_ident == GRUB_UDF_TAG_IDENT_EFE))
     {
Parenthesis are wrong.
+             grub_disk_addr_t sec = grub_udf_get_block(node->data,
+                                                       node->part_ref,
+                                                       ad->position);
ad->position isn't byte-swapped.
ChangeLog entry?
> Greetings,
> -- Giuseppe Caizzone <acaizzo@gmail.com>
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]