Re: [Libcdio-devel] libcdio tools cannot read multi-exent files with Joliet exensions on

[Rocky Bernstein]

Comments in line.

On Fri, Jun 24, 2022 at 8:40 AM Thomas Schmitt <scdbackup@gmx.net> wrote:

> Hi,
>
> Ben Kohler wrote:
> > I've found that for an iso
> > created with (cdrtools) mkisofs -J -iso-level 3, libcdio tools like
> > iso-info and iso-read are not able to handle multi-extent files.
>
> It's a pair of bugs in function _iso9660_dir_to_statbuf() of libcdio
> source file lib/iso9660/iso9660_fs.c .
>
> -----------------------------------------------------------------------
> Symptoms:
>
> I can reproduce this with a xorriso-made ISO too, which has Rock Ridge
> additionally to Joliet.
> (mkisofs with -R doesn't help either. genisoimage is safe because it
> does not obey -iso-level 3 and aborts already on image production.)
>
> A workaround for iso-info seems to be option
>   --no-joliet
> man iso-read does not mention this option.
>
> A run of iso-info without --no-joliet complains:
>
>   ++ WARN: Non consecutive multiextent file parts for ''
>
> -----------------------------------------------------------------------
> Diagnosis:
>
> The message comes from _iso9660_dir_to_statbuf() in iso9660_fs.c.
> It is supposed to report a filename in the quotation marks.
> In order to get there, a re-used p_stat has to be submitted by the
> caller.
>
> The first bug is this:
>
> When iso9660_ifs_readdir() calls _iso9660_dir_to_statbuf() with the first
> directory entry of the multi-extent file it gets back an empty string as
> file name, because the conversion from UCS-2 to UTF-8 is missing if the
> ISO_MULTIEXTENT bit is set in the directory record.
> This is probably my fault due to the assumption that libcdio already
> deals with C strings at that point of processing.
> See
>
> https://git.savannah.gnu.org/cgit/libcdio.git/commit/?id=d758fa2253cca062e92ad0754a64c15a854db4ff
>   "Use the plain ISO-9660 name when dealing with a multiextent file part"
>
> The second bug is similar:
>
> The test which leads to the warning message compares the C string which
> holds
> the previously read file name with the UCS-2 array which holds the name of
> the follow-up directory record.
> So even if the UTF-8 conversion missing in above bug is done, this test
> still
> detects that the string length does not match the array byte size and thus
> throws the warning and prevents the proper merging of both directory
> records.
> See
>
> https://git.savannah.gnu.org/cgit/libcdio.git/commit/?id=d758fa2253cca062e92ad0754a64c15a854db4ff
>   "Only resolve the full filename when we're not dealing with extent"
>
> -----------------------------------------------------------------------
> Code to test:
>
> I now have an ugly contraption by which iso-info recognizes a multi-extent
> file even if Joliet is present. The base is an outdated repo clone.
> I will have to get a fresh git state and try to find my cheat sheet
> which describes how to submit changes to libcdio.
>
> The current diff (minus all my debugging printfs) is:
>
> diff --git a/lib/iso9660/iso9660_fs.c b/lib/iso9660/iso9660_fs.c
> index be693c7..a84b9fc 100644
> --- a/lib/iso9660/iso9660_fs.c
> +++ b/lib/iso9660/iso9660_fs.c
> @@ -866,8 +866,25 @@ _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir,
>    if ((p_iso9660_dir->file_flags & ISO_MULTIEXTENT) == 0) {
>      /* Check if this is the last part of a multiextent file */
>      if (!first_extent) {
> -      if (strlen(p_stat->filename) != i_fname ||
> -          strncmp(p_stat->filename, &p_iso9660_dir->filename.str[1],
> i_fname) != 0) {
> +      cdio_utf8_t *p_psz_out = NULL;
> +      int bad_multi;
> +
> +#ifdef HAVE_JOLIET
> +      if (u_joliet_level) {
> +       int i_inlen = i_fname;
> +       if (!cdio_charset_to_utf8(&p_iso9660_dir->filename.str[1], i_inlen,
> +                             &p_psz_out, "UCS-2BE")) {
> +          goto fail;
> +        }
> +      } else
> +#endif /*HAVE_JOLIET*/
> +      {
> +        p_psz_out = calloc(1, i_fname + 1);
> +       strncpy (p_psz_out, &p_iso9660_dir->filename.str[1], i_fname);
> +      }
> +      bad_multi = (strcmp(p_stat->filename, p_psz_out) != 0);
> +      free(p_psz_out);
> +      if (bad_multi) {
>         cdio_warn("Non consecutive multiextent file parts for '%s'",
>                   p_stat->filename);
>         goto fail;
> @@ -916,7 +933,22 @@ _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir,
>      }
>    } else {
>        /* Use the plain ISO-9660 name when dealing with a multiextent file
> part */
> -      strncpy(p_stat->filename, &p_iso9660_dir->filename.str[1], i_fname);
> +#ifdef HAVE_JOLIET
> +      if (u_joliet_level) {
> +       int i_inlen = i_fname;
> +       cdio_utf8_t *p_psz_out = NULL;
> +       if (cdio_charset_to_utf8(&p_iso9660_dir->filename.str[1], i_inlen,
> +                             &p_psz_out, "UCS-2BE")) {
> +          strncpy(p_stat->filename, p_psz_out, i_fname);
> +          free(p_psz_out);
> +        } else {
> +          goto fail;
> +        }
> +      } else
> +#endif /*HAVE_JOLIET*/
> +      {
> +       strncpy (p_stat->filename, &p_iso9660_dir->filename.str[1],
> i_fname);
> +      }
>    }
>
>    iso9660_get_dtime(&(p_iso9660_dir->recording_time), true,
> &(p_stat->tm));
>
> --------------------------------------------------------------------------
>
> @ Ben Kohler:
> You'd do me a big favor if you could test this with your use cases.
>
> @ All:
> I am unhappy with the code repetition in above patch (plus the original
> conversion code which is already _iso9660_dir_to_statbuf()).
> Abstraction proposals to unify the now three cdio_charset_to_utf8()
> occasions in _iso9660_dir_to_statbuf() would be welcome.
>
>
Thomas: thanks again for the great work and followup. My suggestion is to
get your patches in now. Then in a week Pete can have something that
matches what is in git master.  I will try to come up with an abstraction
proposal or something to remove redundancy when I get a chance, but that
won't be in the next two weeks as I'll be busy and out of town.

Thanks.


>
> Have a nice day :)
>

Same to you :o)

>
> Thomas
>
>
>