Cleanup on bug 3894 - Fuzzing crashes for SDL_LoadWAV
authorSam Lantinga
Sun, 09 Jun 2019 12:46:10 -0700
changeset 12809572f29f98da0
parent 12808 8a4b1beb4f6e
child 12810 77707a081153
Cleanup on bug 3894 - Fuzzing crashes for SDL_LoadWAV

Simon Hug

Attached is a minor cleanup patch. It changes the option name of one hint to something better, puts one or two more checks in, and adds explicit casting where warnings could appear otherwise.

I hope the naming of the hints and their options is acceptable. It would be kind of awkward to change them after they get released with an official SDL version.
include/SDL_hints.h
src/audio/SDL_wave.c
src/audio/SDL_wave.h
     1.1 --- a/include/SDL_hints.h	Sat Jun 08 19:12:05 2019 -0700
     1.2 +++ b/include/SDL_hints.h	Sun Jun 09 12:46:10 2019 -0700
     1.3 @@ -1135,8 +1135,8 @@
     1.4   *
     1.5   *  This variable can be set to the following values:
     1.6   *
     1.7 - *    "chunksearch"  - Use the RIFF chunk size as a boundary for the chunk search
     1.8 - *    "ignorezero"   - Like "chunksearch", but a zero size searches up to 4 GiB (default)
     1.9 + *    "force"        - Always use the RIFF chunk size as a boundary for the chunk search
    1.10 + *    "ignorezero"   - Like "force", but a zero size searches up to 4 GiB (default)
    1.11   *    "ignore"       - Ignore the RIFF chunk size and always search up to 4 GiB
    1.12   *    "maximum"      - Search for chunks until the end of file (not recommended)
    1.13   */
     2.1 --- a/src/audio/SDL_wave.c	Sat Jun 08 19:12:05 2019 -0700
     2.2 +++ b/src/audio/SDL_wave.c	Sun Jun 09 12:46:10 2019 -0700
     2.3 @@ -28,7 +28,7 @@
     2.4  #endif
     2.5  #ifndef INT_MAX
     2.6  /* Make a lucky guess. */
     2.7 -#define INT_MAX (SDL_MAX_SINT32)
     2.8 +#define INT_MAX SDL_MAX_SINT32
     2.9  #endif
    2.10  #endif
    2.11  
    2.12 @@ -40,17 +40,18 @@
    2.13  #include "SDL_wave.h"
    2.14  
    2.15  /* Reads the value stored at the location of the f1 pointer, multiplies it
    2.16 - * with the second argument, and then stores it back to f1 again.
    2.17 - * Returns SDL_TRUE if the multiplication overflows, f1 does not get modified.
    2.18 + * with the second argument and then stores the result to f1.
    2.19 + * Returns 0 on success, or -1 if the multiplication overflows, in which case f1
    2.20 + * does not get modified.
    2.21   */
    2.22 -static SDL_bool
    2.23 -MultiplySize(size_t *f1, size_t f2)
    2.24 +static int
    2.25 +SafeMult(size_t *f1, size_t f2)
    2.26  {
    2.27      if (*f1 > 0 && SIZE_MAX / *f1 <= f2) {
    2.28 -        return SDL_TRUE;
    2.29 +        return -1;
    2.30      }
    2.31      *f1 *= f2;
    2.32 -    return SDL_FALSE;
    2.33 +    return 0;
    2.34  }
    2.35  
    2.36  typedef struct ADPCM_DecoderState
    2.37 @@ -333,9 +334,9 @@
    2.38  MS_ADPCM_CalculateSampleFrames(WaveFile *file, size_t datalength)
    2.39  {
    2.40      WaveFormat *format = &file->format;
    2.41 -    const size_t blockheadersize = file->format.channels * 7;
    2.42 +    const size_t blockheadersize = (size_t)file->format.channels * 7;
    2.43      const size_t availableblocks = datalength / file->format.blockalign;
    2.44 -    const size_t blockframebitsize = file->format.bitspersample * file->format.channels;
    2.45 +    const size_t blockframebitsize = (size_t)file->format.bitspersample * file->format.channels;
    2.46      const size_t trailingdata = datalength % file->format.blockalign;
    2.47  
    2.48      if (file->trunchint == TruncVeryStrict || file->trunchint == TruncStrict) {
    2.49 @@ -374,9 +375,9 @@
    2.50  {
    2.51      WaveFormat *format = &file->format;
    2.52      WaveChunk *chunk = &file->chunk;
    2.53 -    const size_t blockheadersize = format->channels * 7;
    2.54 +    const size_t blockheadersize = (size_t)format->channels * 7;
    2.55      const size_t blockdatasize = (size_t)format->blockalign - blockheadersize;
    2.56 -    const size_t blockframebitsize = format->bitspersample * format->channels;
    2.57 +    const size_t blockframebitsize = (size_t)format->bitspersample * format->channels;
    2.58      const size_t blockdatasamples = (blockdatasize * 8) / blockframebitsize;
    2.59      const Sint16 presetcoeffs[14] = {256, 0, 512, -256, 0, 0, 192, 64, 240, 0, 460, -208, 392, -232};
    2.60      size_t i, coeffcount;
    2.61 @@ -393,7 +394,7 @@
    2.62      }
    2.63  
    2.64      if (format->bitspersample != 4) {
    2.65 -        return SDL_SetError("Invalid MS ADPCM bits per sample of %d", (int)format->bitspersample);
    2.66 +        return SDL_SetError("Invalid MS ADPCM bits per sample of %u", (unsigned int)format->bitspersample);
    2.67      }
    2.68  
    2.69      /* The block size must be big enough to contain the block header. */
    2.70 @@ -607,10 +608,10 @@
    2.71  
    2.72      while (blockframesleft > 0) {
    2.73          for (c = 0; c < channels; c++) {
    2.74 -            if (nybble & 0x8000) {
    2.75 +            if (nybble & 0x4000) {
    2.76                  nybble <<= 4;
    2.77              } else if (blockpos < blocksize) {
    2.78 -                nybble = state->block.data[blockpos++] | 0x8000;
    2.79 +                nybble = state->block.data[blockpos++] | 0x4000;
    2.80              } else {
    2.81                  /* Out of input data. Drop the incomplete frame and return. */
    2.82                  state->output.pos = outpos - c;
    2.83 @@ -661,7 +662,7 @@
    2.84  
    2.85      state.blocksize = file->format.blockalign;
    2.86      state.channels = file->format.channels;
    2.87 -    state.blockheadersize = state.channels * 7;
    2.88 +    state.blockheadersize = (size_t)state.channels * 7;
    2.89      state.samplesperblock = file->format.samplesperblock;
    2.90      state.framesize = state.channels * sizeof(Sint16);
    2.91      state.ddata = file->decoderdata;
    2.92 @@ -674,7 +675,7 @@
    2.93  
    2.94      /* The output size in bytes. May get modified if data is truncated. */
    2.95      outputsize = (size_t)state.framestotal;
    2.96 -    if (MultiplySize(&outputsize, state.framesize)) {
    2.97 +    if (SafeMult(&outputsize, state.framesize)) {
    2.98          return SDL_OutOfMemory();
    2.99      } else if (outputsize > SDL_MAX_UINT32 || state.framestotal > SIZE_MAX) {
   2.100          return SDL_SetError("WAVE file too big");
   2.101 @@ -737,8 +738,8 @@
   2.102  IMA_ADPCM_CalculateSampleFrames(WaveFile *file, size_t datalength)
   2.103  {
   2.104      WaveFormat *format = &file->format;
   2.105 -    const size_t blockheadersize = format->channels * 4;
   2.106 -    const size_t subblockframesize = format->channels * 4;
   2.107 +    const size_t blockheadersize = (size_t)format->channels * 4;
   2.108 +    const size_t subblockframesize = (size_t)format->channels * 4;
   2.109      const size_t availableblocks = datalength / format->blockalign;
   2.110      const size_t trailingdata = datalength % format->blockalign;
   2.111  
   2.112 @@ -792,18 +793,18 @@
   2.113  {
   2.114      WaveFormat *format = &file->format;
   2.115      WaveChunk *chunk = &file->chunk;
   2.116 -    const size_t blockheadersize = format->channels * 4;
   2.117 +    const size_t blockheadersize = (size_t)format->channels * 4;
   2.118      const size_t blockdatasize = (size_t)format->blockalign - blockheadersize;
   2.119 -    const size_t blockframebitsize = format->bitspersample * format->channels;
   2.120 +    const size_t blockframebitsize = (size_t)format->bitspersample * format->channels;
   2.121      const size_t blockdatasamples = (blockdatasize * 8) / blockframebitsize;
   2.122  
   2.123      /* Sanity checks. */
   2.124  
   2.125 -    /* IMA ADPCAM can also have 3-bit samples, but it's not supported by SDL at this time. */
   2.126 +    /* IMA ADPCM can also have 3-bit samples, but it's not supported by SDL at this time. */
   2.127      if (format->bitspersample == 3) {
   2.128          return SDL_SetError("3-bit IMA ADPCM currently not supported");
   2.129      } else if (format->bitspersample != 4) {
   2.130 -        return SDL_SetError("Invalid IMA ADPCM bits per sample of %d", (int)format->bitspersample);
   2.131 +        return SDL_SetError("Invalid IMA ADPCM bits per sample of %u", (unsigned int)format->bitspersample);
   2.132      }
   2.133  
   2.134      /* The block size is required to be a multiple of 4 and it must be able to
   2.135 @@ -1054,7 +1055,7 @@
   2.136  
   2.137      state.channels = file->format.channels;
   2.138      state.blocksize = file->format.blockalign;
   2.139 -    state.blockheadersize = state.channels * 4;
   2.140 +    state.blockheadersize = (size_t)state.channels * 4;
   2.141      state.samplesperblock = file->format.samplesperblock;
   2.142      state.framesize = state.channels * sizeof(Sint16);
   2.143      state.framestotal = file->sampleframes;
   2.144 @@ -1066,7 +1067,7 @@
   2.145  
   2.146      /* The output size in bytes. May get modified if data is truncated. */
   2.147      outputsize = (size_t)state.framestotal;
   2.148 -    if (MultiplySize(&outputsize, state.framesize)) {
   2.149 +    if (SafeMult(&outputsize, state.framesize)) {
   2.150          return SDL_OutOfMemory();
   2.151      } else if (outputsize > SDL_MAX_UINT32 || state.framestotal > SIZE_MAX) {
   2.152          return SDL_SetError("WAVE file too big");
   2.153 @@ -1137,7 +1138,7 @@
   2.154  
   2.155      /* Standards Update requires this to be 8. */
   2.156      if (format->bitspersample != 8) {
   2.157 -        return SDL_SetError("Invalid companded bits per sample of %d", (int)format->bitspersample);
   2.158 +        return SDL_SetError("Invalid companded bits per sample of %u", (unsigned int)format->bitspersample);
   2.159      }
   2.160  
   2.161      /* Not going to bother with weird padding. */
   2.162 @@ -1222,12 +1223,12 @@
   2.163      }
   2.164  
   2.165      sample_count = (size_t)file->sampleframes;
   2.166 -    if (MultiplySize(&sample_count, format->channels)) {
   2.167 +    if (SafeMult(&sample_count, format->channels)) {
   2.168          return SDL_OutOfMemory();
   2.169      }
   2.170  
   2.171      expanded_len = sample_count;
   2.172 -    if (MultiplySize(&expanded_len, sizeof(Sint16))) {
   2.173 +    if (SafeMult(&expanded_len, sizeof(Sint16))) {
   2.174          return SDL_OutOfMemory();
   2.175      } else if (expanded_len > SDL_MAX_UINT32 || file->sampleframes > SIZE_MAX) {
   2.176          return SDL_SetError("WAVE file too big");
   2.177 @@ -1269,7 +1270,7 @@
   2.178              if (exponent > 0) {
   2.179                  mantissa |= 0x10;
   2.180              }
   2.181 -            mantissa = mantissa << 4 | 0x8;
   2.182 +            mantissa = (mantissa << 4) | 0x8;
   2.183              if (exponent > 1) {
   2.184                  mantissa <<= exponent - 1;
   2.185              }
   2.186 @@ -1281,7 +1282,7 @@
   2.187          while (i--) {
   2.188              Uint8 nibble = ~src[i];
   2.189              Sint16 mantissa = nibble & 0xf;
   2.190 -            Uint8 exponent = nibble >> 4 & 0x7;
   2.191 +            Uint8 exponent = (nibble >> 4) & 0x7;
   2.192              Sint16 step = 4 << (exponent + 1);
   2.193  
   2.194              mantissa = (0x80 << exponent) + step * mantissa + step / 2 - 132;
   2.195 @@ -1315,11 +1316,11 @@
   2.196              /* These are supported. */
   2.197              break;
   2.198          default:
   2.199 -            return SDL_SetError("%d-bit PCM format not supported", (int)format->bitspersample);
   2.200 +            return SDL_SetError("%u-bit PCM format not supported", (unsigned int)format->bitspersample);
   2.201          }
   2.202      } else if (format->encoding == IEEE_FLOAT_CODE) {
   2.203          if (format->bitspersample != 32) {
   2.204 -            return SDL_SetError("%d-bit IEEE floating-point format not supported", (int)format->bitspersample);
   2.205 +            return SDL_SetError("%u-bit IEEE floating-point format not supported", (unsigned int)format->bitspersample);
   2.206          }
   2.207      }
   2.208  
   2.209 @@ -1353,12 +1354,12 @@
   2.210      Uint8 *ptr;
   2.211  
   2.212      sample_count = (size_t)file->sampleframes;
   2.213 -    if (MultiplySize(&sample_count, format->channels)) {
   2.214 +    if (SafeMult(&sample_count, format->channels)) {
   2.215          return SDL_OutOfMemory();
   2.216      }
   2.217  
   2.218      expanded_len = sample_count;
   2.219 -    if (MultiplySize(&expanded_len, sizeof(Sint32))) {
   2.220 +    if (SafeMult(&expanded_len, sizeof(Sint32))) {
   2.221          return SDL_OutOfMemory();
   2.222      } else if (expanded_len > SDL_MAX_UINT32 || file->sampleframes > SIZE_MAX) {
   2.223          return SDL_SetError("WAVE file too big");
   2.224 @@ -1422,7 +1423,7 @@
   2.225      }
   2.226  
   2.227      outputsize = (size_t)file->sampleframes;
   2.228 -    if (MultiplySize(&outputsize, format->blockalign)) {
   2.229 +    if (SafeMult(&outputsize, format->blockalign)) {
   2.230          return SDL_OutOfMemory();
   2.231      } else if (outputsize > SDL_MAX_UINT32 || file->sampleframes > SIZE_MAX) {
   2.232          return SDL_SetError("WAVE file too big");
   2.233 @@ -1444,8 +1445,8 @@
   2.234      const char *hint = SDL_GetHint(SDL_HINT_WAVE_RIFF_CHUNK_SIZE);
   2.235  
   2.236      if (hint != NULL) {
   2.237 -        if (SDL_strcmp(hint, "chunksearch") == 0) {
   2.238 -            return RiffSizeChunkSearch;
   2.239 +        if (SDL_strcmp(hint, "force") == 0) {
   2.240 +            return RiffSizeForce;
   2.241          } else if (SDL_strcmp(hint, "ignore") == 0) {
   2.242              return RiffSizeIgnore;
   2.243          } else if (SDL_strcmp(hint, "ignorezero") == 0) {
   2.244 @@ -1517,6 +1518,11 @@
   2.245      /* Data is no longer valid after this function returns. */
   2.246      WaveFreeChunkData(chunk);
   2.247  
   2.248 +    /* Error on overflows. */
   2.249 +    if (SDL_MAX_SINT64 - chunk->length < chunk->position || SDL_MAX_SINT64 - 8 < nextposition) {
   2.250 +        return -1;
   2.251 +    }
   2.252 +
   2.253      /* RIFF chunks have a 2-byte alignment. Skip padding byte. */
   2.254      if (chunk->length & 1) {
   2.255          nextposition++;
   2.256 @@ -1687,7 +1693,7 @@
   2.257          return SDL_SetError("Invalid fact chunk in WAVE file");
   2.258      }
   2.259  
   2.260 -    /* Check the issues common to all encodings. Some unsupported formats set
   2.261 +    /* Check for issues common to all encodings. Some unsupported formats set
   2.262       * the bits per sample to zero. These fall through to the 'unsupported
   2.263       * format' error.
   2.264       */
   2.265 @@ -1763,7 +1769,7 @@
   2.266              const Uint32 g3 = g[6] | ((Uint32)g[7] << 8);
   2.267              return SDL_SetError(errstr, g1, g2, g3, g[8], g[9], g[10], g[11], g[12], g[13], g[14], g[15]);
   2.268          }
   2.269 -        return SDL_SetError("Unknown WAVE format tag: 0x%04x", (int)format->encoding);
   2.270 +        return SDL_SetError("Unknown WAVE format tag: 0x%04x", (unsigned int)format->encoding);
   2.271      }
   2.272  
   2.273      return 0;
   2.274 @@ -1837,7 +1843,7 @@
   2.275              break;
   2.276          }
   2.277          /* fallthrough */
   2.278 -    case RiffSizeChunkSearch:
   2.279 +    case RiffSizeForce:
   2.280          RIFFend = RIFFchunk.position + RIFFchunk.length;
   2.281          RIFFlengthknown = SDL_TRUE;
   2.282          break;
   2.283 @@ -1850,7 +1856,7 @@
   2.284       * chunks. Ignore the chunks we don't know as per specification. This
   2.285       * currently also ignores cue, list, and slnt chunks.
   2.286       */
   2.287 -    while (RIFFend > chunk->position + chunk->length + (chunk->length & 1)) {
   2.288 +    while ((Uint64)RIFFend > (Uint64)chunk->position + chunk->length + (chunk->length & 1)) {
   2.289          /* Abort after too many chunks or else corrupt files may waste time. */
   2.290          if (chunkcount++ >= chunkcountlimit) {
   2.291              return SDL_SetError("Chunk count in WAVE file exceeds limit of %u", chunkcountlimit);
   2.292 @@ -1910,7 +1916,7 @@
   2.293           * all required chunks were found.
   2.294           */
   2.295          if (file->trunchint == TruncVeryStrict) {
   2.296 -            if (RIFFend < chunk->position + chunk->length) {
   2.297 +            if ((Uint64)RIFFend < (Uint64)chunk->position + chunk->length) {
   2.298                  return SDL_SetError("RIFF size truncates chunk");
   2.299              }
   2.300          } else if (fmtchunk.fourcc == FMT && datachunk.fourcc == DATA) {
   2.301 @@ -1938,8 +1944,8 @@
   2.302          /* data chunk is handled later. */
   2.303          if (chunk->fourcc != DATA && chunk->length > 0) {
   2.304              Uint8 tmp;
   2.305 -            Sint64 position = chunk->position + chunk->length - 1;
   2.306 -            if (SDL_RWseek(src, position, RW_SEEK_SET) != position) {
   2.307 +            Uint64 position = (Uint64)chunk->position + chunk->length - 1;
   2.308 +            if (position > SDL_MAX_SINT64 || SDL_RWseek(src, (Sint64)position, RW_SEEK_SET) != (Sint64)position) {
   2.309                  return SDL_SetError("Could not seek to WAVE chunk data");
   2.310              } else if (SDL_RWread(src, &tmp, 1, 1) != 1) {
   2.311                  return SDL_SetError("RIFF size truncates chunk");
   2.312 @@ -2058,7 +2064,7 @@
   2.313              break;
   2.314          default:
   2.315              /* Just in case something unexpected happened in the checks. */
   2.316 -            return SDL_SetError("Unexpected %d-bit PCM data format", format->bitspersample);
   2.317 +            return SDL_SetError("Unexpected %u-bit PCM data format", (unsigned int)format->bitspersample);
   2.318          }
   2.319          break;
   2.320      }
     3.1 --- a/src/audio/SDL_wave.h	Sat Jun 08 19:12:05 2019 -0700
     3.2 +++ b/src/audio/SDL_wave.h	Sun Jun 09 12:46:10 2019 -0700
     3.3 @@ -103,10 +103,10 @@
     3.4  /* Controls how the size of the RIFF chunk affects the loading of a WAVE file. */
     3.5  typedef enum WaveRiffSizeHint {
     3.6      RiffSizeNoHint,
     3.7 -    RiffSizeChunkSearch,
     3.8 +    RiffSizeForce,
     3.9      RiffSizeIgnoreZero,
    3.10      RiffSizeIgnore,
    3.11 -    RiffSizeMaximum,
    3.12 +    RiffSizeMaximum
    3.13  } WaveRiffSizeHint;
    3.14  
    3.15  /* Controls how a truncated WAVE file is handled. */
    3.16 @@ -115,7 +115,7 @@
    3.17      TruncVeryStrict,
    3.18      TruncStrict,
    3.19      TruncDropFrame,
    3.20 -    TruncDropBlock,
    3.21 +    TruncDropBlock
    3.22  } WaveTruncationHint;
    3.23  
    3.24  /* Controls how the fact chunk affects the loading of a WAVE file. */
    3.25 @@ -124,7 +124,7 @@
    3.26      FactTruncate,
    3.27      FactStrict,
    3.28      FactIgnoreZero,
    3.29 -    FactIgnore,
    3.30 +    FactIgnore
    3.31  } WaveFactChunkHint;
    3.32  
    3.33  typedef struct WaveFile