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

[Rocky Bernstein]

Thanks for traking this down.

When I do valgrind testing, I just configure with:

   configure --disabled-shared

It is not elegant but it works.

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.
>
>
> ==========================================================================
>
> 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
>
>
>