libcdio-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Schmitt
Subject: [Libcdio-devel] [RFC] Fixing code flaws around CD-TEXT
Date: Mon, 25 Jun 2018 19:58:43 +0200

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]