Fix bad buffer ID checks in AL.bufQueue - #10466
Conversation
JS object keys are always strings, testing if (bufId !== 0) would always yield true.
| @@ -1450,7 +1450,7 @@ var LibraryOpenAL = { | |||
| if (srcLen > 0.0) { | |||
| var frequency; | |||
| for (var bufId in src.bufQueue) { | |||
There was a problem hiding this comment.
hmm, is this even the right loop? these look like arrays of ints, so we should do of instead of in in modern JS. For backwards compatibility we can do for (var i = 0; i < src.bufQueue.length; i++) etc., but that's larger...
The current loops are confusing, but not horrible I guess.
There was a problem hiding this comment.
It does work, but agreed, if there are people who imbue arrays with custom properties, this has the problem of also iterating over them.
I am hesitant to go changing any behavior for this now though, since not focused on AL at the moment (and I think this is a bit deprecated-ish library anyway as it is non-multithreading friendly)
|
Sorry to revive this 6 year old issue, but I don't think this fix works because the JS string "0" is also truthy so this is always truthy both before and after this fix. I have proposed fix, with a test in #27420. |
When `AL_SAMPLE_OFFSET` or `AL_BYTE_OFFSET` is set on a source, the
library looks up the format of the queued audio data to convert the
offset into seconds for `sourceSeek`.
Previously, this iteration used `for (var bufId in src.bufQueue)` and
checked `if (bufId)`. Because `for..in` iterates over array index
strings (`"0"`, `"1"`, etc.), `bufId` received `"0"` on the first
iteration. In JavaScript, non-empty strings are truthy, so `if (bufId)`
evaluated `if ("0")`, which is always true.
As a result, the loop never skipped index 0. If `src.bufQueue[0]`
happened to be the default placeholder dummy zero buffer (which has
`frequency: 0`), it would read `frequency = 0` and cause a division by
zero (`value /= 0`), failing range validation with `AL_INVALID_VALUE`.
This change replaces both loops in `src/lib/libopenal.js` with `for (var
buf of src.bufQueue)` and checks `if (buf.id !== 0)`. A new test case is
added to `test/openal/test_openal_error.c` to verify this behavior.
The original bug was introduced in fcf8d12.
It seems that there was an attempt to fix this in #10466 but I think
this code always yield true both before and after this change because
"0" string is truthy.
3 participants
JS object keys are always strings, testing if (bufId !== 0) would always yield true.