Hi,
i created another new branch: ts-cdtext-fix
It is about some code flaws which i noticed.
The first two are obvious.
The third one is more riddling, because the existing code makes no sense
and seems to work only by accident.
We could need a real binary file which works with real consumers of .CUE
files if given as argument of CDTEXTFILE.
--------------------------------------------------------------------------
Change 1: dd44e33
Expanded maximum CD-TEXT payload size to 8 full text blocks of 256 * 18 bytes
http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-cdtext-fix&id=dd44e33a2756cdf6780e5aaab1f42dbfc8b60da9
For what reason ever, the internal maximum size only accounted for 2 text
blocks with maximum number of text packs. But there can be 8 blocks, each
representing a different language.
---
I tested that the new code works with the two files
test/data/cdtext-libburnia.cdt
test/data/cdtext.cdt
which do not exceed the old limit.
(I should ponder about how to create a maximum size text pack array.)
==========================================================================
--- a/example/cdtext-raw.c
+++ b/example/cdtext-raw.c
@@ -38,7 +38,8 @@
#include <cdio/cdio.h>
-#define CDTEXT_LEN_BINARY_MAX 9216
+/* Maximum CD-TEXT payload: 8 text blocks with 256 packs of 18 bytes each */
+#define CDTEXT_LEN_BINARY_MAX (8 * 256 * 18)
static void
print_cdtext_track_info(cdtext_t *p_cdtext, track_t i_track, const char
*psz_msg) {
@@ -94,16 +95,21 @@ static cdtext_t *
read_cdtext(const char *path) {
FILE *fp;
size_t size;
- uint8_t cdt_data[CDTEXT_LEN_BINARY_MAX+4];
+ uint8_t *cdt_data = NULL;
cdtext_t *cdt;
+ cdt_data = calloc(CDTEXT_LEN_BINARY_MAX + 4, 1);
+ if (NULL == cdt_data) {
+ fprintf(stderr, "could not allocate memory for cdt_data buffer\n");
+ exit(4);
+ }
fp = fopen(path, "rb");
if (NULL == fp) {
fprintf(stderr, "could not open file `%s'\n", path);
exit(3);
}
- size = fread(cdt_data, sizeof(uint8_t), sizeof(cdt_data), fp);
+ size = fread(cdt_data, 1, CDTEXT_LEN_BINARY_MAX + 4, fp);
fclose(fp);
if (size < 5) {
@@ -128,6 +134,7 @@ read_cdtext(const char *path) {
exit(2);
}
+ free(cdt_data);
return cdt;
}
diff --git a/lib/driver/cdtext_private.h b/lib/driver/cdtext_private.h
index 83cb0db..a907bee 100644
--- a/lib/driver/cdtext_private.h
+++ b/lib/driver/cdtext_private.h
@@ -29,7 +29,9 @@
typedef enum {
- CDTEXT_LEN_BINARY_MAX = 9216,
+ CDTEXT_LEN_BINARY_MAX = 8 * 256 * 18, /* Maximum CD-TEXT payload:
+ 8 blocks, 256 packs, 18 bytes
+ */
CDTEXT_LEN_TEXTDATA = 12,
CDTEXT_LEN_PACK = 18,
CDTEXT_LEN_BLOCKSIZE = 36,
diff --git a/lib/driver/image/bincue.c b/lib/driver/image/bincue.c
index a779fed..c47734f 100644
--- a/lib/driver/image/bincue.c
+++ b/lib/driver/image/bincue.c
@@ -357,17 +357,28 @@ 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[CDTEXT_LEN_BINARY_MAX+4];
+ uint8_t *cdt_data = NULL;
int size;
CdioDataSource_t *source;
char *dirname = cdio_dirname(psz_cue_name);
char *psz_filename = cdio_abspath(dirname, psz_field);
+ cdt_data = calloc(CDTEXT_LEN_BINARY_MAX + 4, 1);
+ if (NULL == cdt_data) {
+ cdio_log(log_level,
+ "%s line %d: cannot allocate memory for CD-TEXT data buffer",
+ psz_cue_name, i_line);
+ free(psz_filename);
+ free(dirname);
+ goto err_exit;
+ }
+
if(NULL == (source = cdio_stdio_new(psz_filename))) {
cdio_log(log_level, "%s line %d: can't open file `%s' for reading",
psz_cue_name, i_line, psz_field);
free(psz_filename);
free(dirname);
+ free(cdt_data);
goto err_exit;
}
size = cdio_stream_read(source, cdt_data, CDTEXT_LEN_BINARY_MAX, 1);
@@ -378,6 +389,7 @@ parse_cuefile (_img_private_t *cd, const char *psz_cue_name)
psz_cue_name, i_line, psz_filename);
free(psz_filename);
free(dirname);
+ free(cdt_data);
free(source);
goto err_exit;
}
@@ -402,6 +414,7 @@ parse_cuefile (_img_private_t *cd, const char *psz_cue_name)
cdio_stdio_destroy (source);
free(psz_filename);
free(dirname);
+ free(cdt_data);
}
} else {
goto format_error;
==========================================================================
--------------------------------------------------------------------------
Change 2: 53d38c7
Applied the proper destructor to CdioDataSource_t if parse_cuefile() sees
an undersized CDTEXTFILE
http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-cdtext-fix&id=53d38c7063c84c09c157441aad97934f603ef99f
(The "free()" line sticked out by having a TAB character.)
It appears inappropirate to simply free CdioDataSource_t without
freeing possible entrails. lib/driver/_cdio_stdio.h declares
/*!
Deallocate resources assocaited with obj. After this obj is unusable.
*/
void cdio_stdio_destroy(CdioDataSource_t *p_obj);
---
Currently i do not know how to test bincue.c with CDTEXTFILE.
The change can be justfied by pointing to line 414 of bincue.c where this
destructor is already used.
==========================================================================
--- a/lib/driver/image/bincue.c
+++ b/lib/driver/image/bincue.c
@@ -390,7 +390,7 @@ parse_cuefile (_img_private_t *cd, const char *psz_cue_name)
free(psz_filename);
free(dirname);
free(cdt_data);
- free(source);
+ cdio_stdio_destroy (source);
goto err_exit;
}
==========================================================================
--------------------------------------------------------------------------
Change 3: 60ec267
Enabled recognition and skipping of MMC headers before raw CD-TEXT data.
http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=ts-cdtext-fix&id=60ec2678d8f51196f4ec1e793ce55f7f99be9e7d
The following code and comment in cdtext-raw.c make no sense:
/* Truncate header when it is too large. The standard is ambiguous here*/
if (cdt_data[0] > 0x80) {
size -= 4;
}
...
if(0 != cdtext_data_init(cdt, cdt_data, size)) {
Reducing size by 4 would match the byte count of an MMC header for CD-TEXT.
But since the header would be prepended, this would not skip the header
but just appease the test in cdtext_data_init() for unaligned data:
if (i_data < CDTEXT_LEN_PACK || 0 != i_data % CDTEXT_LEN_PACK) {
cdio_warn("CD-Text size is too small or not a multiple of pack size");
return -1;
}
The test makes no sense either. If the cdt_data would contain an MMC header,
then cdt_data[0] > 0x80 would indicate a byte count >= 33024 which is very
near to the maximum of 36864.
So this is not a good test for an MMC header.
As it is now, the size reduction by 4 would be triggered if the text packs
would not begin by a pack of type 0x80 "Title". All other pack types are
larger than 0x80.
The test data in
test/data/cdtext.cdt
begin by 0x80 and thus do not get their size reduced by 4.
But if the file begins by 0x81 = "Names of Performers" then the program run
fails
$ cp test/data/cdtext.cdt test.cdt
$ echo -n $'\x81' | dd of=test.cdt bs=1 count=1 conv=notrunc
...
$ example/cdtext-raw test.cdt
++ WARN: CD-Text size is too small or not a multiple of pack size
failed to parse CD-Text file `test.cdt'
$ echo $?
2
The same gesture can be seen in
lib/driver/image/bincue.c
Question is whether any traditional CDTEXTFILE which is referred to by
a .CUE file has appended 4 invalid bytes which get announced by a text pack
of type > 0x81. (This condition sounds weird.)
What might make sense is an attempt to recognize an MMC header and
to skip it when handing the CD-TEXT data to cdtext_data_init().
Since i am unsure about bincue.c and have no example data, i refrain from
making the same change there.
---
I tested by writing the matching size 0x06C2 (= decimal 1728 + 2) and two
zeros before a copy of test/data/cdtext.cdt (which has 1729 bytes due to
a trailing zero):
$ echo -n $'\x06\xC2' >test.cdt
$ dd if=/dev/zero bs=1 count=2 >>test.cdt
...
$ cat test/data/cdtext.cdt >>test.cdt
$ example/cdtext-raw test.cdt
NOTE: Skipped 4 bytes of apparent MMC header.
Language 0 'English':
... expected text output ...
The note message on stderr does not appear with original cdtext.cdt.
It also does not appear if the MMC header bears the wrong size:
$ echo -n $'\x06\x40' >test.cdt
$ dd if=/dev/zero bs=1 count=2 >>test.cdt
...
$ cat test/data/cdtext.cdt >>test.cdt
$ example/cdtext-raw test.cdt
++ WARN: CD-Text size is too small or not a multiple of pack size
failed to parse CD-Text file `test.cdt'
==========================================================================
diff --git a/example/cdtext-raw.c b/example/cdtext-raw.c
index af21db6..38405a0 100644
--- a/example/cdtext-raw.c
+++ b/example/cdtext-raw.c
@@ -37,6 +37,7 @@
#endif
#include <cdio/cdio.h>
+#include <cdio/mmc.h>
/* Maximum CD-TEXT payload: 8 text blocks with 256 packs of 18 bytes each */
#define CDTEXT_LEN_BINARY_MAX (8 * 256 * 18)
@@ -95,8 +96,9 @@ static cdtext_t *
read_cdtext(const char *path) {
FILE *fp;
size_t size;
- uint8_t *cdt_data = NULL;
+ uint8_t *cdt_data = NULL, *cdt_packs;
cdtext_t *cdt;
+ int mmc_len;
cdt_data = calloc(CDTEXT_LEN_BINARY_MAX + 4, 1);
if (NULL == cdt_data) {
@@ -117,9 +119,20 @@ read_cdtext(const char *path) {
exit(1);
}
- /* Truncate header when it is too large. The standard is ambiguous here*/
- 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;
+ fprintf(stderr, "NOTE: Skipped 4 bytes of apparent MMC header.\n");
+ }
}
/* ignore trailing 0 */
@@ -128,7 +141,7 @@ read_cdtext(const char *path) {
/* init cdtext */
cdt = cdtext_init ();
- if(0 != cdtext_data_init(cdt, cdt_data, size)) {
+ if(0 != cdtext_data_init(cdt, cdt_packs, size)) {
fprintf(stderr, "failed to parse CD-Text file `%s'\n", path);
free(cdt);
exit(2);
==========================================================================
Have a nice day :)
Thomas