[Libcdio-devel] Shall we rectify the defintion of iso_su_ce_t for 64 bit alignment ?

[Thomas Schmitt]

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