[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT
From: |
Thomas Schmitt |
Subject: |
Re: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT |
Date: |
Tue, 26 Jun 2018 15:13:59 +0200 |
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