[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT
From: |
Rocky Bernstein |
Subject: |
Re: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT |
Date: |
Tue, 26 Jun 2018 09:30:13 -0400 |
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
>
>
>