Bugzilla – Full Text Bug Listing
| Summary: | Ray#length() can result in integer overflow | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Alex Boyko <aboyko> | ||||||
| Component: | GEF-Legacy Draw2d | Assignee: | Alex Boyko <aboyko> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | ahunter.eclipse, hudsonr | ||||||
| Version: | 3.4 | Keywords: | contributed | ||||||
| Target Milestone: | 3.4.1 (Ganymede SR1) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
The patch looks fine. Should be be upgrading all the methods which square integers?
I don't think we can change the signature due to binary compatibility, but we could add a new method:
long dotProductL(Ray) {
//return long dotProduct using code from patch
}
And call the new method from length()
Randy, About upgrading methods to which square integers. There are 2 places where we calculate length: 1) Point#getDistance(Point) (Point#getDistance2(Point)- this is already fixed. 2) Ray#dotProduct(Ray) - will be fixed here as you suggested. Other places (I assumed they could only be in "geometry" package) I found only in Ray class i.e. similarity(Ray) assimilarity(Ray) methods. The methods are being called for "direction" vectors (i.e. (0,1), (-1,0) etc.). If this is how methods meant to be used, then perhaps I don't need to introduce the new ones returning long? What's your opinion? Now Point#getDistance2(Point), there is private double Point#getPreciseDistance2(Point) used by getDistance(Point). I looked at places from where getDistance2(Point) is being called. All calls compare the result of the call with a small int (result < smallInt), so we don't care if there is an overflow. So, it's the same story as similarity assimilarity from Ray class, the context either doesn't care about overflow or it's impossible. So, I'm not sure what to do in these 2 cases... What's your opinion? It sounds like those methods aren't a risk based on how GEF is using them, so it really just boils down to what clients are doing. It just seems like there is a constant trickle of defects about precision or overflow, but if no one is having problems (or maybe they are using Point instead), then no further changes are needed. Probably existing clients wouldn't notice such a new method anyway unless they came across the overflow issue. Created attachment 111844 [details]
patch
Patch to commit.
Can we commit this for the maintenance release too?
Committed to HEAD and R3_4_maintenance |
Created attachment 111729 [details] proposed patch Ray#length() uses the result from Ray#dotProduct(Ray) which returns an int. Hence if x == 0 and y == Integer.MAX_VALUE an overflow will occur and length won't be equal to Integer.MAX_VALUE. The proposed patch is attached