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
