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

[Thomas Schmitt]

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