Hi,
i need opinions about resolving a possible alignment problem with
using members of iso_su_ce_t as input of function from_733(), which
expects uint64_t.
Different from the other SUSP representing structs, its 2x32-bit numbers
are declared as uint8_t array[8] rather than as iso733_t, which is
actually uint64_t.
Shouldn't we change in include/cdio/rock.h
typedef struct iso_su_ce_s {
uint8_t extent[8];
uint8_t offset[8];
uint8_t size[8];
} iso_su_ce_t;
to
typedef struct iso_su_ce_s {
iso733_t extent;
iso733_t offset;
iso733_t size;
} GNUC_PACKED iso_su_ce_t;
to avoid any alignment problems with from_733() ?
The current macro definition CHECK_CE is broken anyways (see below).
It could be adapted to the new declaration of iso_su_ce_t by simply
removing the '*' operators, so that it looks like:
#define CHECK_CE(FAIL) \
{ cont_extent = from_733(rr->u.CE.extent); \
cont_offset = from_733(rr->u.CE.offset); \
if (cont_offset >= ISO_BLOCKSIZE) FAIL; \
cont_size = from_733(rr->u.CE.size); \
if (cont_size >= ISO_BLOCKSIZE) FAIL; \
}
I tested this with iso-info and it seems to work well.
---------------------------------------------------------------------
Long story:
After getting an applicable patch from Pete Batard, i found a new
harmless problem and an old nasty one:
- A surplus semicolon in the new definition of CONTINUE_DECLS causes
with gcc:
rock.c: In function 'get_rock_ridge_filename':
rock.c:180:3: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
int i_namelen = 0;
^
- The old '*' operators in the calls to from_733() in the definition of
CHECK_CE truncate the numbers of the CE entry to single-byte values.
If the values are actually larger than 255, this leads to a wrong read
address in the loaded continuation area block. In the end iso-info
fails with a SIGSEGV in print_fs_attrs() because
p_statbuf->rr.psz_symlink is NULL.
Possible fixes are:
- Either remove the new semicolon at the end of the definition of
CONTINUE_DECLS or remove the old semicolon at the invocation of
CONTINUE_DECLS.
- Change in the definition of CHECK_CE the gesture
from_733(*rr->u.CE.xxxxx);
to
from_733(*((uint64_t *) rr->u.CE.xxxxx));
The first fix is unproblematic.
But the second one could still invite trouble with 64-bit alignment.
In practice the function parameter declared in include/cdio/bytesex.h is
not used as its type but immediately converted back to a byte array:
static CDIO_INLINE uint32_t
from_733 (uint64_t p)
{
uint8_t *u = (uint8_t *) &p;
/* Return the little-endian part always, to handle non-specs-compliant
images */
return (u[0] | (u[1] << 8) | (u[2] << 16) | (u[3] << 24));
}
gcc seems to create stable code from this on a 64-bit x86 machine.
But we do not know what ideas future compilers might try to apply.
After all this code uses the address of a potentially non-aligned
uint64_t.
In lib/iso9660/iso9660.c the input to from_733() is surely aligned,
because in the end it is declared as iso733_t which is typedef uint64_t.
But in lib/iso9660/rock.c CE_CHECK it gets fed by uint8_t array[8], which
is not guaranteed to be aligned to 64 bit:
typedef struct iso_su_ce_s {
uint8_t extent[8];
uint8_t offset[8];
uint8_t size[8];
} iso_su_ce_t;
Other structs of which members are input to from_733() are:
iso_rock_px_s, iso_rock_pn_s, iso_rock_cl_s
Their submitted elements are all iso733_t.
So changing struct iso_su_ce_s to:
typedef struct iso_su_ce_s {
iso733_t extent;
iso733_t offset;
iso733_t size;
} GNUC_PACKED iso_su_ce_t;
would bring iso_su_ce_t nearer to other SUSP struct definitions like:
typedef struct iso_rock_px_s {
iso733_t st_mode; /*! file mode permissions; same as st_mode
of POSIX:5.6.1 */
iso733_t st_nlinks; /*! number of links to file; same as st_nlinks
of POSIX:5.6.1 */
iso733_t st_uid; /*! user id owner of file; same as st_uid
of POSIX:5.6.1 */
iso733_t st_gid; /*! group id of file; same as st_gid of
of POSIX:5.6.1 */
} GNUC_PACKED iso_rock_px_t ;
Have a nice day :)
Thomas