icculus / physfs

A portable, flexible file i/o abstraction.
https://icculus.org/physfs/
zlib License
560 stars 98 forks source link

archive_iso9660 has wrong ISO9660FileFlags struct size when built using MSVC #36

Closed SDLBugzilla closed 2 years ago

SDLBugzilla commented 2 years ago

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: all Reported for operating system, platform: Windows Vista, PC

Comments on the original bug report:

On 2015-04-10 12:34:56 -0400, Jonathan Hamilton wrote:

Created attachment 3532 Fix ISO9660FileFlags struct size on msvc

MSVC (at least version 13 - 'VS Express 2013 for Desktop') appear to expect the minimum size of packed bitfields to be specified. This means using 'unsigned' causes the size of the flags field to be 4 bytes (instead of the expected one).

This then causes the sizeof() the various record structs to be incorrect, causing them to be incorrectly read from the file.

Tested on windows 7 and windows 8.1 built with 'VS Express 2013 for Desktop'

sezero commented 2 years ago

This is https://bugzilla.icculus.org/show_bug.cgi?id=6377 and the patch file linked above is completely wrong. Correct version:

commit 070cc0f6f296a19392f0924db9bfb01fa354bf4b
Author: Jonathan Hamilton <jtrhamilton@gmail.com>
Date:   Thu Apr 9 22:10:41 2015 -0700

    Fix archive_iso9660 reading in msvc

    ISO9660FileFlags was the wrong size, as msvc expects the max size of bitfield types to be specified - set to uint8 instead of unsigned so it's correctly 1 byte.
    Without this, it would fail to read the archive as the headers would be read mis-aligned

diff --git a/src/archiver_iso9660.c b/src/archiver_iso9660.c
index e7d5362..60427c3 100644
--- a/src/archiver_iso9660.c
+++ b/src/archiver_iso9660.c
@@ -84,17 +88,21 @@ typedef struct
     PHYSFS_sint8 offset;
 } ISO9660FileTimestamp;

 typedef struct
 {
-  unsigned existence:1;
-  unsigned directory:1;
-  unsigned associated_file:1;
-  unsigned record:1;
-  unsigned protection:1;
-  unsigned reserved:2;
-  unsigned multiextent:1;
+  PHYSFS_uint8 existence:1;
+  PHYSFS_uint8 directory : 1;
+  PHYSFS_uint8 associated_file : 1;
+  PHYSFS_uint8 record:1;
+  PHYSFS_uint8 protection : 1;
+  PHYSFS_uint8 reserved : 2;
+  PHYSFS_uint8 multiextent : 1;
 } ISO9660FileFlags;

 typedef struct
 {
     PHYSFS_uint8 length;
icculus commented 2 years ago

patch file linked above is completely wrong

Fixed link in original comment, sorry about that!

icculus commented 2 years ago

We changed this at some point to read individual fields and not entire structs, solving this issue.