Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vector.take(Int.MaxValue) fails because of integer overflow #9581

Closed
scabug opened this issue Dec 6, 2015 · 5 comments
Closed

Vector.take(Int.MaxValue) fails because of integer overflow #9581

scabug opened this issue Dec 6, 2015 · 5 comments
Assignees
@ruippeixotog
Milestone
2.11.8

Comments

@scabug
Copy link

scabug commented Dec 6, 2015

take in (Vector.fill(1)("a") ++ Vector.fill(64)("a")).take(Int.MaxValue) fails, with 63 for 64 it succeeds. This is minimized from this Twitter report: https://twitter.com/travisbrown/status/673331342273150976

The issue starts at the integer overflow in this comparison:
https://github.com/scala/scala/blob/v2.11.7/src/library/scala/collection/immutable/Vector.scala#L159

For this case, I claimed on Twitter that using unsignedCompare(Int, Int) (see e.g. http://www.drmaciver.com/2008/08/unsigned-comparison-in-javascala/ for one clever implementation) would suffice as a fix — but I've not verified no loopholes remain.

It's not clear to me why dropBack0 fails there, but it's safe to assume it wasn't designed to handle negative arguments. I'm hoping the result of ++ is still kosher here, but I don't have the hybris of reviewing the code in the time I have.

Travis Brown tested this on 2.11, I did on 2.11.7, but the overflowing comparing was already there in 2009, and I'm assuming since always. Would you please test other maintained branches? scala/scala@6f7723b#diff-c6f1c361902458cf90978d39ae61cfa8L128
@scabug
Copy link
Author

scabug commented Dec 6, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9581?orig=1
Reporter: @Blaisorblade
Affected Versions: 2.11.7

@scabug
Copy link
Author

scabug commented Dec 6, 2015

@soc said:
I think we could also use https://docs.oracle.com/javase/8/docs/api/java/lang/Integer.html#compareUnsigned-int-int-

@scabug
Copy link
Author

scabug commented Dec 6, 2015

@Blaisorblade said:
Since that method is new with JDK 1.8, we can't use it for 2.11 — 2.11.8 seems still planned [1]. But I'd missed that Scala 2.12 will require Java 8 anyway [2], so we should use it there.
This assumes the release roadmaps haven't changed.

[1] http://www.scala-lang.org/news/2.11.7
[2] http://www.scala-lang.org/news/2.12-roadmap

@scabug
Copy link
Author

scabug commented Dec 6, 2015

@ruippeixotog said:
I submitted a fix for this in scala/scala#4876. As I explain there, changing the comparison you mention to an equivalent one is sufficient in this case to handle all argument values. I think there is no need to use any special unsigned comparison method :)

@scabug scabug closed this as completed Dec 15, 2015
@scabug
Copy link
Author

scabug commented Dec 15, 2015

@SethTisue said:
re "Would you please test other maintained branches", the fix and the test you added will be merged onto the 2.12.x branch sometime in the next few weeks

@scabug scabug added this to the 2.11.8 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ruippeixotog ruippeixotog

Labels
None yet
Projects
None yet
Milestone
2.11.8
Development

No branches or pull requests
2 participants
@ruippeixotog @scabug