webkitAudioContext does not do proper sanity checking on its arguments.

• Component: DOM
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Chromium https://code.google.com/p/chromium/issues/detail?id=88638
Split off from bug 63997.

webkitAudioContext takes three arguments, which should be non-zero positive values that are limited by the amount of available memory. The code is currently not doing proper checks, which causes crashes when you supply invalid arguments:

Repro:
<script>
  // This should not crash but simply output "PASS".
  var aiArgs = [
      [-1,1,1], [0,1,1],
      [1,-1,1], [1,0,1],
      [1,1,-1], [1,1,0],
      [0x7FFFFFFF, 0x7FFFFFFF, 0x7FFFFFFF]],
      bFail = false;
  if (!window.webkitAudioContext) {
    document.write('DISABLED');
  } else {
    for (var i = 0; i < aiArgs.length; i++) {
      var code = 'new webkitAudioContext(' + aiArgs[i].join(',') + ')';
      try { eval(code); }
      catch (e) { continue; }
      document.write('FAIL: ' + code);
      bFail = true;
    }
    if (!bFail) document.write('PASS');
  }
</script>

[Berend-Jan Wever]

See also bug 63997

[Chris Rogers]

Created attachment 100193 [details]
Patch

[WebKit Review Bot]

Comment on attachment 100193 [details]
Patch

Attachment 100193 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8997835

[Kenneth Russell]

Comment on attachment 100193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100193&action=review

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:78
> +        signed numberOfChannels = exec->argument(0).toInt32(exec);
> +        signed numberOfFrames = exec->argument(1).toInt32(exec);

Why not use int32 rather than signed?

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:135
> +    signed numberOfChannels = exec->argument(0).toInt32(exec);
> +    signed numberOfFrames = exec->argument(1).toInt32(exec);

Same here.

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:70
> +        signed numberOfChannels = toInt32(args[0], ok);

Same here.

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:74
> +        signed numberOfFrames = toInt32(args[1], ok);

Same here.

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:133
> +    signed numberOfChannels = toInt32(args[0], ok);

And here.

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:137
> +    signed numberOfFrames = toInt32(args[1], ok);

And here.

> Source/WebCore/webaudio/AudioContext.cpp:91
> +PassRefPtr<AudioContext> AudioContext::createOfflineContext(Document* document, unsigned numberOfChannels, size_t numberOfFrames, double sampleRate, ExceptionCode& ec)

Why is numberOfChannels still unsigned if the caller is passing signed? Similar for numberOfFrames.

[Kenneth Russell]

Comment on attachment 100193 [details]
Patch

On second thought I'm r-'ing this.

[Chris Rogers]

Created attachment 100199 [details]
Patch

[Chris Rogers]

Fixed signed -> int32_t

The actual API is unsigned, but it seems useful to do bounds checking on the parameters as signed, since negative values can be easily dealt with.

[WebKit Review Bot]

Comment on attachment 100199 [details]
Patch

Attachment 100199 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8999738

[Chris Rogers]

Created attachment 100200 [details]
Patch

[Chris Rogers]

Sorry - last patch fixes cr-linux compile error

[Kenneth Russell]

Comment on attachment 100200 [details]
Patch

OK, looks good.

[Chris Rogers]

Committed r90839: <http://trac.webkit.org/changeset/90839>