(CLJS) Out-of-bounds index values passed in timers

Description

In the cljs.core.async.impl.timers namespace, there are places where an aget call is made with an out-of-bounds index value, with the code instead checking a post-condition that a nil is returned.

(These can be detected at runtime using the new :checked-arrays feature being added to the ClojureScript compiler.)

One specific example is in the SkipList remove implementation where the (aget (.-forward x) 0) call can evidently be made against an empty array (you can see that the code checks for nil as a result of the call instead of checking a precondition that the array has elements).

Another example that I've seen at runtime is the (aget (.-forward x) level) in least-greater-node where evidently level can be either equal to or greater than the length of the array.

Environment

None

Activity

Show:
Mike Fikes
September 21, 2017, 3:35 PM

I've been successfully using the attached patch for development without issue, and with a patched core.async having been deployed into production for about a month (iOS app in the App Store which makes use of timers).

Mike Fikes
July 15, 2017, 2:29 PM

The attached ASYNC-201.patch adds tests for the skip list implementation used in the timers namespace and only makes revisions where they are empirically seen to be needed (in other words, it doesn't blindly add checks before every aget and aset call, as many may be fine).

You in addition to running the tests as-is, you can run them with the new checked arrays feature by temporarily changing the project.clj to specify [org.clojure/clojure "1.8.0"] and a locally-build copy of ClojureScript master, and by adding :checked-arrays :warn to the ClojureScript compiler options specified under :cljsbuld (this key-value pair could be added adjacent to every :static-fns setting. You can also run the tests this way without the patch and you will see warnings being emitted for the skip list implementation by virtue of being driven by the existing test suite.

Completed
Your pinned fields
Click on the next to a field label to start pinning.

Assignee

David Nolen

Reporter

Mike Fikes

Approval

Vetted

Patch

Code and Test