liballeg / allegro5

The official Allegro 5 git repository. Pull requests welcome!
https://liballeg.org
Other
1.9k stars 286 forks source link

Possible errors in voc.c #1341

Open nick7inc opened 2 years ago

nick7inc commented 2 years ago

allegro5/addons/acodec/voc.c

In ALLEGRO_SAMPLE _al_load_voc_f(ALLEGRO_FILE file)

  1. buffer = al_realloc(buffer, sizeof(buffer) + bytestoread); Looks bad. I have no idea how new size are calculated here.
  2. Memory leak in code, the memory for AL_VOC_DATA allocates twice: 1st - in _al_load_voc_f() body, then address of pointer overwrites after voc_open() call.
nick7inc commented 2 years ago

And the last, this file (https://disk.yandex.ru/d/9TUjxRu5Ovjpzw) loads incorrectly (only 1st block)

beoran commented 2 years ago

Thanks for reporting this. A PR with a fix would be most welcome.

nick7inc commented 2 years ago

I am not very strong in programming (in git-specific things I almost zero), so I can only advise. To fix bug with memory leak just remove allocation from _al_load_voc_f() body:

ALLEGRO_SAMPLE *_al_load_voc_f(ALLEGRO_FILE *file)
{
 // I prefer in-place declaration, do not like uninitialised and non-zero pointers, so commenting thins:
  // AL_VOC_DATA *vocdata;
   ALLEGRO_SAMPLE *sample = NULL;
   size_t pos = 0; /* where to write in the buffer */
   size_t read = 0; /*bytes read during last operation */
  // The same - uninitialised pointers are buggy:
   char* buffer=0; size_t buffer_size = 0;

   size_t bytestoread = 0;
   bool endofvoc = false;

// Removing (commenting) unnecessary allocation causes memory leak:
   // vocdata = al_malloc(sizeof(AL_VOC_DATA));
  // memset(vocdata, 0, sizeof(*vocdata));
   /*
    * Open file and populate VOC DATA, then create a buffer for the number of
    * samples of the frst block.
    * Iterate on the following blocks till EOF or terminator block
    */
   AL_VOC_DATA *vocdata = voc_open(file);
   if (!vocdata) return NULL; 
nick7inc commented 2 years ago

With this line:

buffer = al_realloc(buffer, sizeof(buffer) + bytestoread);

It is incorrect - sizeof(buffer) - as buffer is not fixed-size (stack) variable (looks like copy-paste from elsewhere), so this startment returns size of pointer, not a buffer itself. It is requires to use additional variable size_t buffer_size; to store current size of buffer before allocation. I am doing back-port of this function for Allegro4 library and for my own library, so I join functions _al_load_voc_f() and voc_open() and use this kind of code before every next block reading occurs :

if (bytestoread + pos > buffer_size)
                {
                  buffer_size = bytestoread + pos;
                  if (buffer)
                    buffer = (char*) realloc (buffer, buffer_size);
                  else buffer = (char*) malloc (buffer_size);
                  if (!buffer)
                    return RS_Out_of_memory; // In case of Allegro5 library - return NULL;
                }

Atantion! In code of functions _al_load_voc_f() and voc_open() are used variables with different names to specify next block length, so check if bytestoread are used or variable with another name before insert this code.

nick7inc commented 2 years ago

About 3d error (see my VOC example link in previous message), I found this specification: https://fabiensanglard.net/reverse_engineering_strike_commander/docs/Creative%20Voice%20(VOC)%20file%20format.txt As I understand, voc can consist of multiple blocks of sound (but not only) data. In my example (I took it from DukeNukem Atomic GRP file, there are a lot of voc inside). In MUZAK028.VOC there are at least 2 blocks of type 1 with sound data (size of each block is about 16384), your library refuses to read multiple blocks type 1, it whats block type 2 after block type 1, and interrupts reading on 2nd block type 1.

nick7inc commented 2 years ago

In voc_open() case 9: looks incomplete. vocdata->channels, vocdata->bits and vocdata->samplerate are filled, but no vocdata->sample_size and vocdata->samples.

nick7inc commented 2 years ago

About 3d error. I have made my backport function with continuous reading block type 1 (with it's header parameters check if they are identical for all blocks). Sound loading success and plays correctly.

nick7inc commented 2 years ago

Backport of _al_load_voc_f() for Allegro 4.2.1. I have replaced packread file I/O interface with my virtual I/O class UFILE, which are a copy of stdio fopen(), so you need to convert back to packread, it is not very difficult. This function reads VOC from DukeNukem3D without problems (original - first block only).


    template <typename T> class AutoFree
    {
      private:
        T* &array_ptr;
      public:
        AutoFree (T*&array_ptr) : array_ptr (array_ptr) {}
        ~AutoFree ()
        {if (array_ptr) {free (array_ptr);array_ptr = 0;} return ;}
    };

  // Backport from Allegro 5 with some fixes (memory leak, rejecting continuous reading set of blocks type 1 and etc.

  struct AL_VOC_DATA
    {
      class UFILE   &file;
      size_t         datapos;
      int            samplerate;
      short          bits;        /* 8 (unsigned char) or 16 (signed short) */
      short          channels;    /* 1 (mono) or 2 (stereo) */
      int            sample_size; /* channels * bits/8 */
      int            samples_per_block, total_samples;     /* # of samples. size = samples * sample_size */
      unsigned int   blocks_readed;
      bool           blank;
      AL_VOC_DATA (class UFILE   &f) :
          file (f),   datapos (0),   samplerate (0),   bits (0),
          channels (0),      sample_size (0),
          samples_per_block (0),total_samples(0),
          blocks_readed(0),
          blank (true)
      {}
    };

  /* Enum: ALLEGRO_AUDIO_DEPTH
   */
  enum ALLEGRO_AUDIO_DEPTH
  {
    /* Sample depth and type, and signedness. Mixers only use 32-bit signed
     * float (-1..+1). The unsigned value is a bit-flag applied to the depth
     * value.
     */
    ALLEGRO_AUDIO_DEPTH_INT8      = 0x00,
    ALLEGRO_AUDIO_DEPTH_INT16     = 0x01,
    ALLEGRO_AUDIO_DEPTH_INT24     = 0x02,
    ALLEGRO_AUDIO_DEPTH_FLOAT32   = 0x03,

    ALLEGRO_AUDIO_DEPTH_UNSIGNED  = 0x08,

    /* For convenience */
    ALLEGRO_AUDIO_DEPTH_UINT8  = ALLEGRO_AUDIO_DEPTH_INT8 |
    ALLEGRO_AUDIO_DEPTH_UNSIGNED,
    ALLEGRO_AUDIO_DEPTH_UINT16 = ALLEGRO_AUDIO_DEPTH_INT16 |
    ALLEGRO_AUDIO_DEPTH_UNSIGNED,
    ALLEGRO_AUDIO_DEPTH_UINT24 = ALLEGRO_AUDIO_DEPTH_INT24 |
    ALLEGRO_AUDIO_DEPTH_UNSIGNED,

    ALLEGRO_AUDIO_DEPTH_UNKNOWN = -1
  };

  /* Enum: ALLEGRO_CHANNEL_CONF
   */
  enum ALLEGRO_CHANNEL_CONF
  {
    /* Speaker configuration (mono, stereo, 2.1, 3, etc). With regards to
     * behavior, most of this code makes no distinction between, say, 4.1 and
     * 5 speaker setups.. they both have 5 "channels". However, users would
     * like the distinction, and later when the higher-level stuff is added,
     * the differences will become more important. (v>>4)+(v&0xF) should yield
     * the total channel count.
     */
    ALLEGRO_CHANNEL_CONF_1   = 0x10,
    ALLEGRO_CHANNEL_CONF_2   = 0x20,
    ALLEGRO_CHANNEL_CONF_3   = 0x30,
    ALLEGRO_CHANNEL_CONF_4   = 0x40,
    ALLEGRO_CHANNEL_CONF_5_1 = 0x51,
    ALLEGRO_CHANNEL_CONF_6_1 = 0x61,
    ALLEGRO_CHANNEL_CONF_7_1 = 0x71,
#define ALLEGRO_MAX_CHANNELS 8
    ALLEGRO_CHANNEL_CONF_UNKNOWN = -1
  };

#define READNBYTES(f, data, n, retv)                                           \
  do {                                                                        \
      if (f.read(&data, 1, n) != n) \
        {                                        \
          return retv;                                                          \
        }                                                                        \
    } while(0)

  /* FIXME: assumes 8-bit is unsigned, and all others are signed. */
  enum ALLEGRO_AUDIO_DEPTH
  _al_word_size_to_depth_conf (int word_size)
  {
    switch (word_size)
      {
        case 1:
          return ALLEGRO_AUDIO_DEPTH_UINT8;
        case 2:
          return ALLEGRO_AUDIO_DEPTH_INT16;
        case 3:
          return ALLEGRO_AUDIO_DEPTH_INT24;
        case 4:
          return ALLEGRO_AUDIO_DEPTH_FLOAT32;
        default:
          return ALLEGRO_AUDIO_DEPTH_UNKNOWN;
      }
  }

  /* FIXME: use the allegro provided helpers */
  enum ALLEGRO_CHANNEL_CONF
  _al_count_to_channel_conf (int num_channels)
  {
    switch (num_channels)
      {
        case 1:
          return ALLEGRO_CHANNEL_CONF_1;
        case 2:
          return ALLEGRO_CHANNEL_CONF_2;
        case 3:
          return ALLEGRO_CHANNEL_CONF_3;
        case 4:
          return ALLEGRO_CHANNEL_CONF_4;
        case 6:
          return ALLEGRO_CHANNEL_CONF_5_1;
        case 7:
          return ALLEGRO_CHANNEL_CONF_6_1;
        case 8:
          return ALLEGRO_CHANNEL_CONF_7_1;
        default:
          return ALLEGRO_CHANNEL_CONF_UNKNOWN;
      }
  }

  Return_status
  _al5_uload_voc_mod (class UFILE &file, SAMPLE * &sample)
  {
    struct AL_VOC_DATA vocdata (file);
    if (sample) destroy_sample(sample);
    sample = 0;
    size_t pos = 0; /* where to write in the buffer */
    size_t buffer_size = file.lof();
    char* buffer = (char*)malloc(buffer_size);
    if (!buffer) return RS_Out_of_memory;
    class AutoFree<char> af1 (buffer);
    zerobuffer(buffer,buffer_size);

    size_t bytestoread = 0;
    bool endofvoc = false;

    /*
     * Open file and populate VOC DATA, then create a buffer for the number of
     * samples of the frst block.
     * Iterate on the following blocks till EOF or terminator block
     */

    {
      // Checking header
      size_t readcount = 0;
      uint16_t vocversion = 0;
      uint16_t checkver = 0;  // must be  1's complement of vocversion + 0x1234

      {
        char hdrbuf[0x16];
        memset (hdrbuf, 0, sizeof (hdrbuf));
        /* Begin checking the Header info */
        readcount = vocdata.file.read (hdrbuf, 1, 0x16);
        if (readcount != 0x16                                       /*short header*/
            || memcmp (hdrbuf, "Creative Voice File\x1A", 0x14) != 0 /*wrong id */
            || memcmp (hdrbuf + 0x14, "\x1A\x00", 0x2) != 0)
          {         /*wrong offset */
            // ALLEGRO_ERROR("voc_open: File does not appear to be a valid VOC file");
            return RS_unknown_format;
          }
      }
      READNBYTES (vocdata.file, vocversion, 2, RS_unexpected_EOF);

      if (vocversion != 0x10A && vocversion != 0x114)
        {   // known ver 1.10 -1.20
//      ALLEGRO_ERROR("voc_open: File is of unknown version");
          return RS_unknown_version;
        }
      /* checksum version check */
      READNBYTES (vocdata.file, checkver, 2, RS_unexpected_EOF);
      if (checkver != ~vocversion + 0x1234)
        {
//      ALLEGRO_ERROR("voc_open: Bad VOC Version Identification Number");
          return RS_unknown_version;
        }
    }

    /*
     * We're at the first datablock, we shall check type and set all the relevant
     * info in the vocdata structure, including finally the datapos index to the
     * first valid data byte.
     */

    /*
     * We now need to iterate over data blocks till either we hit end of file
     * or we find a terminator block.
     */

    while (!endofvoc && (!vocdata.file.eof()))
      {
        uint8_t blocktype = 0;
        uint32_t x = 0; // Must be 32 bit, used as tmp buffer to skip 4 bytes
        uint32_t blocklength = 0; //length is stored in 3 bites LE, gd byteorder.

        READNBYTES (vocdata.file, blocktype, 1, RS_unexpected_EOF);// read next block type
        // vocdata.file.eof() == true at this point if blocktype==0

        blocklength = 0;x = 0;
        if (blocktype != 0) // Terminator Block is an exception -- it has only the TYPE byte.
          {
            READNBYTES (vocdata.file, blocklength, 2, RS_unexpected_EOF);
            if (vocdata.file.eof()) return RS_unexpected_EOF;
            READNBYTES (vocdata.file, x, 1, RS_unexpected_EOF);
            if (vocdata.file.eof()) return RS_unexpected_EOF;
            blocklength += x << 16;
          }

        vocdata.blocks_readed++;
        switch (blocktype)
          {
            case 0:   /* we found a terminator block */
            {
              endofvoc = true;
              break;
            }
            case 2:   /*we found a continuation block: unlikely but handled */
            {
              if (vocdata.blank) return RS_Not_ready; // ALLEGRO_ERROR("voc_open: opening Block is of unsupported type");
              if (blocklength<1) return RS_data_integrity_fail;
//              x = 0;
//              bytestoread = 0;
              bytestoread = blocklength;
              /* increase subsequently storage */
              if (bytestoread + pos > buffer_size)
                {
                  buffer_size = bytestoread + pos;
                  if (buffer)
                    buffer = (char*) realloc (buffer, buffer_size);
                  else buffer = (char*) malloc (buffer_size);
                  if (!buffer)
                    return RS_Out_of_memory;
                }
              const size_t read = vocdata.file.read (buffer + pos, 1, bytestoread);
              vocdata.samples_per_block = blocklength / vocdata.sample_size;
              vocdata.total_samples += vocdata.samples_per_block;
              pos += read;
              break;
            }
            case 1:
            case 8:
            case 9:
            {
              if (!blocklength) return RS_data_integrity_fail;
              uint16_t timeconstant = 0;
              uint16_t format = 0;    // can be 16-bit in Blocktype 9 (voc version > 1.20

              bytestoread = 0;
              switch (blocktype)
                {
                  case 1:
                  {
                    /* blocktype 1 is the most basic header with 1byte format, time
                     * constant and length equal to (datalength + 2).
                     */
                    if (blocklength<3) return RS_data_integrity_fail;
                    blocklength -= 2;
                    READNBYTES (vocdata.file, timeconstant, 1, RS_unexpected_EOF);
                    READNBYTES (vocdata.file, format, 1, RS_unexpected_EOF);
                    if (vocdata.blank)
                      {
                        vocdata.bits = 8; /* only possible codec for Blocktype 1 */
                        vocdata.channels = 1;  /* block 1 alone means MONO */
                        vocdata.samplerate = 1000000 / (256 - timeconstant);
                        vocdata.sample_size = 1; /* or better: 1 * 8 / 8 */
                      }
                    else
                      {
                        if (
                          (vocdata.bits != 8)
                          || (vocdata.channels != 1)
                          || (vocdata.samplerate != 1000000 / (256 - timeconstant))
                          || (vocdata.sample_size != 1)
                        )
                          return RS_Type_Mismatch;
                      }
                    /*
                     * Expected number of samples is at LEAST what is in this block.
                     * IIF lentgh 0xFFF there will be a following blocktype 2.
                     * We will deal with this later in load_voc.
                     */
                    vocdata.samples_per_block = blocklength / vocdata.sample_size;
                    vocdata.total_samples += vocdata.samples_per_block;
                    vocdata.datapos = vocdata.file.tell();//al_ftell(fp);
                    vocdata.blank = false;
                    bytestoread = vocdata.samples_per_block * vocdata.sample_size;
                  }
                  break;

                  case 8:
                  {
                    /* Blocktype 8 is enhanced data block (mainly for Stereo samples I
                     * guess) that precedes a Blocktype 1, of which the sound infor have
                     * to be ignored.
                     * We skip to the end of the following Blocktype 1 once we get all the
                     * required header info.
                     */
                    if (blocklength != 4)
                      {
//            ALLEGRO_ERROR("voc_open: Got opening Blocktype 8 of wrong length");
                        return RS_corrupted_format;
                      }
                    READNBYTES (vocdata.file, timeconstant, 2, RS_unexpected_EOF);
                    READNBYTES (vocdata.file, format, 1, RS_unexpected_EOF);
                    READNBYTES (vocdata.file, vocdata.channels, 1, RS_unexpected_EOF);
                    if (vocdata.blank)
                      {
                        vocdata.bits = 8; /* only possible codec for Blocktype 8 */
                      }
                    else
                      {
                        // vocdata.channels, vocdata.samplerate and vocdata.sample_size can alter
                        if (vocdata.bits != 8)
                          return RS_Type_Mismatch;
                      }
                    vocdata.channels += 1; /* was 0 for mono, 1 for stereo */
                    vocdata.samplerate = 1000000 / (256 - timeconstant);
                    vocdata.samplerate /= vocdata.channels;
                    vocdata.sample_size = vocdata.channels * vocdata.bits / 8;

                    /*
                     * Now following there is a blocktype 1 which tells us the length of
                     * the data block and all other info are discarded.
                     */
                    READNBYTES (vocdata.file, blocktype, 1, RS_unexpected_EOF);
                    if (blocktype != 1)
                      {
//            ALLEGRO_ERROR("voc_open: Blocktype following type 8 is not 1");
                        return RS_corrupted_format;
                      }
                    READNBYTES (vocdata.file, blocklength, 2, RS_unexpected_EOF);
                    x = 0;
                    READNBYTES (vocdata.file, x, 1, RS_unexpected_EOF);
                    blocklength += x << 16;
                    if (blocklength<3) return RS_data_integrity_fail;
                    blocklength -= 2;
                    x = 0;
                    READNBYTES (vocdata.file, x, 2, RS_unexpected_EOF);
                    vocdata.samples_per_block = blocklength / vocdata.sample_size;
                    vocdata.total_samples += vocdata.samples_per_block;
                    vocdata.datapos = vocdata.file.tell();//al_ftell(fp);
                    vocdata.blank = false;
//                  bytestoread = vocdata.samples * vocdata.sample_size;
                    bytestoread = blocklength;
                  }
                  break;

                  case 9:
                  {
                    /*
                     * Blocktype 9 is available only for VOC version 1.20 and above.
                     * Deals with 16-bit codecs and stereo and is richier than blocktype 1
                     * or the BLocktype 8+1 combo
                     * Length is 12 bytes more than actual data.
                     */
                    if (blocklength<=12) return RS_data_integrity_fail;
                    blocklength -= 12;
                    if (vocdata.blank)
                      {
                        READNBYTES (vocdata.file, vocdata.samplerate, 4, RS_unexpected_EOF);  // actual samplerate
                        READNBYTES (vocdata.file, vocdata.bits, 1, RS_unexpected_EOF);        // actual bits
                        // after compression
                        READNBYTES (vocdata.file, vocdata.channels, 1, RS_unexpected_EOF);    // actual channels
          #warning: Not sure about this:
                        vocdata.sample_size = vocdata.channels * vocdata.bits / 8;
                      }
                    else
                      {
                        int samplerate=0,sample_size=0;
                        char bits=0, channels=0;
                        READNBYTES (vocdata.file, samplerate, 4, RS_unexpected_EOF);  // actual samplerate
                        READNBYTES (vocdata.file, bits, 1, RS_unexpected_EOF);        // actual bits
                        // after compression
                        READNBYTES (vocdata.file, channels, 1, RS_unexpected_EOF);    // actual channels
                        sample_size=channels*bits/8;
                        if (
                          (vocdata.samplerate != samplerate)
                          || (vocdata.bits != bits)
                          || (vocdata.channels != channels)
                          || (vocdata.sample_size != sample_size)
                        )
                          return RS_Type_Mismatch;
                      }
                    READNBYTES (vocdata.file, format, 2, RS_unexpected_EOF);
                    if ( (vocdata.bits != 8 && vocdata.bits != 16) ||
                         (format != 0 && format != 4))
                      {
//            ALLEGRO_ERROR("voc_open: unsupported CODEC in voc data");
                        return RS_Feature_not_supported;
                      }
                    x = 0;
                    // Skipping unused (align) bytes:
                    READNBYTES (vocdata.file, x, 4, RS_unexpected_EOF);        // just skip 4 reserved bytes
                    vocdata.datapos = vocdata.file.tell();//al_ftell(fp);
                    vocdata.blank = false;
                    bytestoread = blocklength;
                    vocdata.samples_per_block = blocklength / vocdata.sample_size;
                    vocdata.total_samples +=  vocdata.samples_per_block;
                  }
                  break;
                  default:
                    return RS_Nothing_here;
                    break;
                }

              /*
              * Let's allocate at least the first block's bytes;
              */

              if (bytestoread + pos > buffer_size)
                {
                  buffer_size = bytestoread + pos;
                  if (buffer)
                    buffer = (char*) realloc (buffer, buffer_size);
                  else buffer = (char*) malloc (buffer_size);
                  if (!buffer)
                    return RS_Out_of_memory;
                }
              const size_t read = vocdata.file.read (buffer + pos, 1, bytestoread);
              pos += read;
              break;
            }

            case 3:     /* we found a pause block */
            case 4:     /* we found a marker block */
            case 5:     /* we found an ASCII c-string block */
            case 6:     /* we found a repeat block */
            case 7:     /* we found an end repeat block */
            {
              if (vocdata.blank) return RS_Not_ready; // ALLEGRO_ERROR("voc_open: opening Block is of unsupported type");
              /* all these blocks will be skipped */

              // blocklength can be zero.
              for (unsigned int ii = 0; ii < blocklength ; ++ii)
                {
                  vocdata.file.getc();
                }
              bytestoread = 0;  //should let safely check for the next block */
              break;
            }
            default:
              return RS_unknown_format;
              break;
          }
      }

//   ALLEGRO_DEBUG("channels %d\n", vocdata.channels);
//   ALLEGRO_DEBUG("word_size %d\n", vocdata.sample_size);
//   ALLEGRO_DEBUG("rate %d\n", vocdata.samplerate);
//   ALLEGRO_DEBUG("first_block_samples %d\n", vocdata.samples);
//   ALLEGRO_DEBUG("first_block_size %d\n", vocdata.samples * vocdata.sample_size);

//ALLEGRO_KCM_AUDIO_FUNC(ALLEGRO_SAMPLE *, al_create_sample, (void *buf,
//      unsigned int samples, unsigned int freq, ALLEGRO_AUDIO_DEPTH depth,
//      ALLEGRO_CHANNEL_CONF chan_conf, bool free_buf));
//ALLEGRO_KCM_AUDIO_FUNC(void, al_destroy_sample, (ALLEGRO_SAMPLE *spl));

//   sample = al_create_sample(buffer, pos, vocdata.samplerate,
//                             _al_word_size_to_depth_conf(vocdata.sample_size),
//                             _al_count_to_channel_conf(vocdata.channels),
//      true);
    {
//      const enum ALLEGRO_AUDIO_DEPTH aadepth =
//        _al_word_size_to_depth_conf (vocdata.sample_size);
      const enum ALLEGRO_CHANNEL_CONF aaccf =
        _al_count_to_channel_conf (vocdata.channels);
      const bool not_ok =
        ( (vocdata.bits != 8) && (vocdata.bits != 16))
        || ( (aaccf != ALLEGRO_CHANNEL_CONF_1) && (aaccf != ALLEGRO_CHANNEL_CONF_2));
      if (!not_ok)
        {
          const int stereo = ALLEGRO_CHANNEL_CONF_1 ? FALSE : TRUE;
          const size_t len = pos / vocdata.sample_size;
          sample = create_sample (vocdata.bits, stereo, vocdata.samplerate, len);
          if (sample)
            memcpy (sample->data, buffer, len*vocdata.sample_size);
        }
    }

    return RS_Success;
  }
nick7inc commented 2 years ago
  inline float get_sample_instance_time(const struct SAMPLE *spl)
{
   if (!spl) return 0.0;
   return (float)(spl->len) / (float)spl->freq;
}