[Libcdio-devel] proposed fix for bug #22865

[Pete Batard]

The next issue we probably want to fix at this stage is the one with the 
buffer overflow (bug #22865), which can be displayed by adding some 
instrumentation.

If you apply the attached patch, here is what you get:

(...)
Extracting: /mnt/data/test/sources/inf/setup.cfg
Extracting: /mnt/data/test/sources/input.dll
Extracting: /mnt/data/test/sources/install.exe
++ WARN: MISMATCH! p_udf_dirent = 0x804bd08: i_alloc_desc 40 (new LBA) 
vs 8 (existing)
++ WARN: MISMATCH! p_udf_dirent = 0x804bd08: i_alloc_desc 40 (new LBA) 
vs 8 (existing)
Extracting: /mnt/data/test/sources/install.wim
  Error reading UDF file /sources/install.wim
--DEBUG: closed source...
Segmentation fault

As previously explained, the issue here is that we are trying to reuse a 
previously allocated file entry (struct udf_file_entry_s), that was 
allocated with 8 bytes for alloc_desc, for one that requires 40 bytes.

After giving it a bit more thought, as well as applying something 
similar in my app, my proposal for a fix would be to ensure that struct 
udf_file_entry_s is always 2048 bytes (UDF_BLOCK_SIZE), regardless of 
how many bytes are needed for ext_attr[] and alloc_descs[].

Thus, instead of:

/** File Entry (ECMA 167r3 4/14.9) */
struct udf_file_entry_s
{
  udf_tag_t       tag;
(...)
  udf_Uint64_t    unique_ID;
  udf_Uint32_t    i_extended_attr;
  udf_Uint32_t    i_alloc_descs;
  udf_Uint8_t     ext_attr[0];
  udf_Uint8_t     alloc_descs[0];
} GNUC_PACKED;

We could use:

/** File Entry (ECMA 167r3 4/14.9) */
struct udf_file_entry_s
{
  udf_tag_t       tag;
(...)
  udf_Uint64_t    unique_ID;
  udf_Uint32_t    i_extended_attr;
  udf_Uint32_t    i_alloc_descs;
  union {
    udf_Uint8_t   ext_attr[0];
    udf_Uint8_t   alloc_descs[0];
    udf_Uint8_t   padding[2048-176];
  } u;
} GNUC_PACKED;

And modify the code accordingly. Obviosuly, the 176 comes from the size 
of the structure without the union.

While allocating 2k for each FE may sound wasteful, I don't think it 
will impact much in terms of performance, since we need to allocate one 
block whenever we read an FE anyway. Also, the specs indicate that an FE 
cannot be of greater size than an UDF block, so the upper limit is well 
known, and this leaves us free to reuse FEs without worrying about 
overflows. The other option would be to use a pointer and realloc when 
we need to grow, but I don't think it would be as robust.

Finally, once we start adding MSVC support, we will have a problem with 
zero sized arrays anyway, as you can't have two of them in sequence. 
Using an union with a fixed size element alleviates the issue.

Since I need to redo this patch, I'll wait for comments about this 
proposed approach before submitting one for review.

Regards,

/Pete

diff --git a/lib/udf/udf_fs.c b/lib/udf/udf_fs.c
index dcd6e53..1f53f0a 100644
--- a/lib/udf/udf_fs.c
+++ b/lib/udf/udf_fs.c
@@ -668,6 +668,12 @@ udf_readdir(udf_dirent_t *p_udf_dirent)
                         + p_udf_dirent->fid->icb.loc.lba, 1))
                return NULL;
       
+        if ((p_udf_fe->i_alloc_descs != p_udf_dirent->fe.i_alloc_descs))
+          cdio_warn("MISMATCH! p_udf_dirent = %p: i_alloc_desc %d (new LBA) vs 
%d (existing)",
+                    p_udf_dirent, p_udf_fe->i_alloc_descs, 
p_udf_dirent->fe.i_alloc_descs);
+        if ((p_udf_fe->i_alloc_descs != p_udf_dirent->fe.i_alloc_descs))
+          cdio_warn("MISMATCH! p_udf_dirent = %p: i_alloc_desc %d (new LBA) vs 
%d (existing)",
+                    p_udf_dirent, p_udf_fe->i_alloc_descs, 
p_udf_dirent->fe.i_alloc_descs);
        memcpy(&(p_udf_dirent->fe), p_udf_fe, 
               sizeof(udf_file_entry_t) + p_udf_fe->i_alloc_descs 
               + p_udf_fe->i_extended_attr );