Re: [Libcdio-devel] Fedora bug report concerning CD-TEXT

[Thomas Schmitt]

Hi,

it turned out that the code about reading ISRC is potentially dangerous
because its strdup() relies on MMC-4 specs which promise a 0-byte
in byte 17 of the reply of READ SUB-CHANNEL.

Although i would bet CD semantics on that 0-byte, i would not bet
the memory integrity of the program on it.

So the test whether transfer length 24 yields better results
should be done with this change:

--- lib/driver/mmc/mmc.c.orig   2014-09-25 03:19:42.000000000 +0200
+++ lib/driver/mmc/mmc.c        2016-04-02 08:59:01.285553911 +0200
@@ -551,7 +551,7 @@ mmc_get_track_isrc_private ( void *p_env
                              )
 {
   mmc_cdb_t cdb = {{0, }};
-  char buf[28] = { 0, };
+  char buf[25];
   int i_status;
 
   if ( ! p_env || ! run_mmc_cmd )
@@ -559,6 +559,7 @@ mmc_get_track_isrc_private ( void *p_env
 
   CDIO_MMC_SET_COMMAND(cdb.field, CDIO_MMC_GPCMD_READ_SUBCHANNEL);
   CDIO_MMC_SET_READ_LENGTH8(cdb.field, sizeof(buf));
+  memset(buf, 0, sizeof(buf));
 
   cdb.field[1] = 0x0;
   cdb.field[2] = 0x40;
@@ -568,8 +569,9 @@ mmc_get_track_isrc_private ( void *p_env
   i_status = run_mmc_cmd(p_env, mmc_timeout_ms,
                              mmc_get_cmd_len(cdb.field[0]),
                              &cdb, SCSI_MMC_DATA_READ,
-                             sizeof(buf), buf);
+                             24, buf);
   if(i_status == 0) {
+    buf[24] = 0;
     return strdup(&buf[9]);
   }
   return NULL;

The use of constants 24 and 25 might not match the coding style of
libcdio. So this is only a rough test proposal, not a patch for
inclusion in libcdio.


This was tested by prepending "valgrind" to the program start in
script ./src/cd-info . Result:

  ==29605== LEAK SUMMARY:
  ==29605==    definitely lost: 40 bytes in 3 blocks
  ...
  ==29605== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

With additional valgrind option --leak-check=full the memory leaks
are reported as:

  ==31996== 14 bytes in 1 blocks are definitely lost in loss record 2 of 3
  ==31996==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
  ==31996==    by 0x55EB9D9: strdup (strdup.c:42)
  ==31996==    by 0x504C66D: get_mcn_linux (gnu_linux.c:525)
  ==31996==    by 0x402DEC: main (cd-info.c:1117)
  ==31996== 
  ==31996== 26 bytes in 2 blocks are definitely lost in loss record 3 of 3
  ==31996==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
  ==31996==    by 0x55EB9D9: strdup (strdup.c:42)
  ==31996==    by 0x5055F8E: mmc_get_track_isrc_private (mmc.c:586)
  ==31996==    by 0x402E38: main (cd-info.c:1133)

(Stacks are obviously sparser than in source, due to optimization.)


Have a nice day :)

Thomas