-
Notifications
You must be signed in to change notification settings - Fork 22
Arithmetic overflow possible in IterableOnceOps.copyToArray #12795
Description
Reproduction steps
Scala version: 2.13.10
In IterableOnceOps:
def copyToArray[B >: A](xs: Array[B], start: Int, len: Int): Int = {
val it = iterator
var i = start
val end = start + math.min(len, xs.length - start)
while (i < end && it.hasNext) {
xs(i) = it.next()
i += 1
}
i - start
}Problem
There are actually two overflows here, but xs.length - start is arguably not an issue, as a negative start causes a negative end and nothing will be copied (instead of throwing an exception if an overflow had not happened). There is a second one in start + math.min(len, ...), however, and on a valid path:
copyToArray(new Array[Hamster](Int.MaxValue - 100), Int.MaxValue - 50, Int.MaxValueThe 'new' collection framework is much better in terms of avoiding overflows than the old one, where almost everything was overflowing, but if there was some competition I'm pretty sure I could find a good dozen more. Iterator.drop in complex iterator implementations in particular is very vulnerable, and that's a bit of a bummer, because iterators actually can be easily used for collections with more than Int.MaxValue elements.
I'd like to use this opportunity to climb my soap box and say again I don't like the exact semantics of indices out of range in copyToArray. Essentially, they are closely tied to this particular implementation, and troublesome to reproduce to a tee for all possible data sets. A negative start is allowed only if the total number of elements which would be copied, as influenced by len, this.size, and xs.length, would be zero or less. I think it would be cleaner if it was either always rejected with an exception, or always accepted.
While I can see the value of permissive indices in slicing operations, where we are dealing with bounds on collection sizes, using them in a context of specific indices in a sequence (like indexOfSlice and some other issues I reported),
- doesn't seem to me to follow the policy of least surprise,
- is inconsistent with mutable collections, in which all mutating operations (as far as I can tell), reject all cases of an index out of range (see
remove,insertAll, etc.), - is not particularly useful. For example, if
copyToArrayalways accepted negativestartand simply ignored-startfirst elements in the collection, it would be actually quite useful.
The worst offender, by far, though, is, in my eyes, patch. The policy of clipping the index of the start of the patch, can easily lead to quiet bugs, without any beneficial use cases I can see. If, as I suggested above, instead the indices were not modified, but the part of the elems (patch) collection falling outside of the patched (this) collection was ignored, it would be both more logical and useful.
Ehm, sorry.