Re: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT

[Rocky Bernstein]

On Tue, Jun 26, 2018 at 9:13 AM, Thomas Schmitt <address@hidden> wrote:

> Hi,
>
> i applied an equivalent change as in cdtext-raw.c, after Leon confirmed
> that the gesture in driver/image/bincue.c
>
>         /* Truncate header when it is too large. */
>         if (cdt_data[0] > 0x80) {
>           size -= 4;
>         }
>
> was indeed intended to handle an MMC header which may be prepended to
> the text pack array.
>
> See at end of mail or in
>   http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-cdtext-fix&id=
> 388e859770724155b13da7fbdce7bfbb65adb104
>
> For testing, i faked a .CUE file which is intended to match
> test/data/cdtext.cdt.
> test/data/cdtextfile.cue :
>
>   CDTEXTFILE "cdtext.cdt"
>   FILE "cdtextfile.bin" BINARY
>   TRACK 01 AUDIO
>     INDEX 00 00:00:00
>   TRACK 02 AUDIO
>     INDEX 00 00:01:00
>     INDEX 01 00:01:05
>   TRACK 03 AUDIO
>     INDEX 00 00:03:00
>     INDEX 01 00:03:05
>
> A program which refers to .cue files is test/testpregap.c. It uses
> cdio_open(,DRIVER_UNKNOWN). On my clone it is not compiled by default:
>   $ cd test
>   $ make testpregap
>     CC       testpregap.o
>     CCLD     testpregap
>   $
>
> I added cdtextfile.cue to its list of test images.
> It demands "cdtextfile.bin" when i run it. So i copied
>   $ cp data/cdda.bin data/cdtextfile.bin
> Interesting error messages appear if track 2 gets less than 2 seconds of
> length.
>
> But i want my own errors. So i spoiled the file name in cdtextfile.cue:
>   CDTEXTFILE "Xcdtext.cdt"
> which yields
>   cdio 3 message: could not retrieve file info for 
> `.../libcdio/test/data/Xcdtext.cdt':
> No such file or directory
>
> -------------------------------------------------------------------------
> Now for testing my code changes in bincue.c:
> -------------------------------------------------------------------------
> The corrected destructor call if file size < 5:
> http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-cdtext-fix&id=
> 53d38c7063c84c09c157441aad97934f603ef99f
>
> I changed in cdtextfile.cue
>   CDTEXTFILE "bad_cdtext.cdt"
> with
>   $ dd if=/dev/zero bs=4 count=1 of=data/bad_cdtext.cdt
> yields:
>   cdio 3 message: .../libcdio/test/data/cdtextfile.cue line 1: file
> `.../libcdio/test/data/bad_cdtext.cdt' is too small to contain CD-TEXT
> which is then supposed to be followed by the destructor call.
>
> No SIGSEGV.
>
> If somebody knows how to use valgrind in the pseudo-binaries like
> ./testpregap, then please teach me.
>
> -------------------------------------------------------------------------
> The freshly changed recognition and skipping of MMC header:
>
>   $ echo -n $'\x06\xC2' >test/data/cdtext_w_head.cdt
>   $ dd if=/dev/zero bs=1 count=2 >>data/cdtext_w_head.cdt
>   ...
>   $ cat data/cdtext.cdt >>data/cdtext_w_head.cdt
> and in cdtextfile.cue
>   CDTEXTFILE "cdtext_w_head.cdt"
> yields:
>   cdio 3 message: .../libcdio/test/data/cdtextfile.cue line 1: skipped 4
> bytes of apparent MMC header in CD-TEXT file `.../libcdio/test/data/cdtext_
> w_head.cdt'
>
> No SIGSEGV or protests from CD-TEXT parsing.
>
> (I had to raise the log_level of the message to CDIO_LOG_WARN because
>  else testpregap does not report it. But i think that CDIO_LOG_INFO is the
>  appropriate level for production.)
>
> -------------------------------------------------------------------------
>
> I will think about whether and how above two experiments should become
> part of libcdio.
> Corrently it is a problem for me that testpregap wants a dedicated .bin
> file for each .cue file. This would mean two or three *.bin of 700 KB each.
>


This was discussed a long while ago with respect to vcdimager. The solution
we hit on then was to have
another package of test images which could be quite large. If those were
installed in your filesystem additional tests would be run.

I can't remember where that project is or what happened to the old test
images.



>
>
> ==========================================================================
>
> diff --git a/lib/driver/image/bincue.c b/lib/driver/image/bincue.c
> index f51beb4..68ed73c 100644
> --- a/lib/driver/image/bincue.c
> +++ b/lib/driver/image/bincue.c
> @@ -357,8 +357,8 @@ parse_cuefile (_img_private_t *cd, const char
> *psz_cue_name)
>    } else if (0 == strcmp ("CDTEXTFILE", psz_keyword)) {
>      if(NULL != (psz_field = strtok (NULL, "\"\t\n\r"))) {
>        if (cd) {
> -        uint8_t *cdt_data = NULL;
> -        int size;
> +        uint8_t *cdt_data = NULL, *cdt_packs;
> +        int size, mmc_len;
>          CdioDataSource_t *source;
>          char *dirname = cdio_dirname(psz_cue_name);
>          char *psz_filename = cdio_abspath(dirname, psz_field);
> @@ -394,9 +394,22 @@ parse_cuefile (_img_private_t *cd, const char
> *psz_cue_name
> )
>            goto err_exit;
>          }
>
> -        /* Truncate header when it is too large. */
> -        if (cdt_data[0] > 0x80) {
> -          size -= 4;
> +        /* Check whether obviously a MMC header is prepended and if
> so,skip it.
> +           cdtext_data_init() wants to see only the text pack bytes.
> +        */
> +        cdt_packs = cdt_data;
> +        if (cdt_data[0] < 0x80) {
> +          /* This cannot be a text pack start */
> +          mmc_len = CDIO_MMC_GET_LEN16(cdt_data) + 2;
> +          if ((size == mmc_len || size == mmc_len + 1) && mmc_len % 18 ==
> 4 &&
> +              cdt_data[4] >= 0x80 && cdt_data[4] <= 0x8f) {
> +            /* It looks much like a MMC header followed by a text pack
> start */
> +            size -= 4;
> +            cdt_packs = cdt_data + 4;
> +            cdio_log (CDIO_LOG_INFO,
> +                      "%s line %d: skipped 4 bytes of apparent MMC header
> in CD-TEXT file `%s'\n",
> +                      psz_cue_name, i_line, psz_filename);
> +          }
>          }
>
>          /* ignore trailing 0 */
> @@ -408,7 +421,7 @@ parse_cuefile (_img_private_t *cd, const char
> *psz_cue_name)
>            cd->gen.cdtext = cdtext_init ();
>          }
>
> -        if(0 != cdtext_data_init(cd->gen.cdtext, cdt_data, size))
> +        if(0 != cdtext_data_init(cd->gen.cdtext, cdt_packs, size))
>            cdio_log (log_level, "%s line %d: failed to parse CD-TEXT file
> `%s'", psz_cue_name, i_line, psz_filename);
>
>          cdio_stdio_destroy (source);
>
> ==========================================================================
>
> Have a nice day :)
>
> Thomas
>
>
>