In java/awt/font/TextJustifier.java justify-method there is a potential code path where divison by zero might happen , see also the Sonar finding
https://sonarcloud.io/project/issues?id=shipilev_jdk&open=AXcqMwpm8sPJZZzONu1k&resolved=false&severities=CRITICAL&types=BUG boolean hitLimit = (weight == 0) || (!lastPass && ((delta < 0) == (delta < gslimit))); boolean absorbing = hitLimit && absorbweight > 0; // predivide delta by weight float weightedDelta = delta / weight; // not used if weight == 0 In case of (weight == 0) the division should not be done because the value of weightedDelta is unused in this case anyway. ------------- Commit messages: - JDK-8263362 Changes: https://git.openjdk.java.net/jdk/pull/2912/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2912&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263362 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2912.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2912/head:pull/2912 PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Wed, 10 Mar 2021 12:31:31 GMT, Matthias Baesken <[hidden email]> wrote:
> In java/awt/font/TextJustifier.java justify-method there is a potential code path where divison by zero might happen , see also the Sonar finding > https://sonarcloud.io/project/issues?id=shipilev_jdk&open=AXcqMwpm8sPJZZzONu1k&resolved=false&severities=CRITICAL&types=BUG > > > boolean hitLimit = (weight == 0) || (!lastPass && ((delta < 0) == (delta < gslimit))); > boolean absorbing = hitLimit && absorbweight > 0; > // predivide delta by weight > float weightedDelta = delta / weight; // not used if weight == 0 > > In case of (weight == 0) the division should not be done because the value of weightedDelta is unused in this case anyway. src/java.desktop/share/classes/java/awt/font/TextJustifier.java line 159: > 157: // predivide delta by weight > 158: float weightedDelta = 0; > 159: if (weight != 0) { // not used if weight == 0 Can it ever be -ve? Maybe we can do weight > 0 check just as we do for absorbweight? ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Wed, 10 Mar 2021 12:41:05 GMT, Prasanta Sadhukhan <[hidden email]> wrote:
>> In java/awt/font/TextJustifier.java justify-method there is a potential code path where divison by zero might happen , see also the Sonar finding >> https://sonarcloud.io/project/issues?id=shipilev_jdk&open=AXcqMwpm8sPJZZzONu1k&resolved=false&severities=CRITICAL&types=BUG >> >> >> boolean hitLimit = (weight == 0) || (!lastPass && ((delta < 0) == (delta < gslimit))); >> boolean absorbing = hitLimit && absorbweight > 0; >> // predivide delta by weight >> float weightedDelta = delta / weight; // not used if weight == 0 >> >> In case of (weight == 0) the division should not be done because the value of weightedDelta is unused in this case anyway. > > src/java.desktop/share/classes/java/awt/font/TextJustifier.java line 159: > >> 157: // predivide delta by weight >> 158: float weightedDelta = 0; >> 159: if (weight != 0) { // not used if weight == 0 > > Can it ever be -ve? Maybe we can do weight > 0 check just as we do for absorbweight? Hi, I am not sure about the weight > 0 check ; weight is initialized with 0: weight = 0; and later some values are potentially added up to weight: weight += gi.weight; I am not sure about those gi.weight values, maybe they can be negative too ? ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Wed, 10 Mar 2021 12:55:34 GMT, Matthias Baesken <[hidden email]> wrote:
>> src/java.desktop/share/classes/java/awt/font/TextJustifier.java line 159: >> >>> 157: // predivide delta by weight >>> 158: float weightedDelta = 0; >>> 159: if (weight != 0) { // not used if weight == 0 >> >> Can it ever be -ve? Maybe we can do weight > 0 check just as we do for absorbweight? > > Hi, I am not sure about the weight > 0 check ; weight is initialized with 0: weight = 0; and later some values are potentially added up to weight: weight += gi.weight; > I am not sure about those gi.weight values, maybe they can be negative too ? Nothing throws an exception or otherwise prevent this being negative but that would be a weird usage. Typically the weight is either zero or based on the font size .. which ought not to be negative but I don't think anything prevents it and I think we would treat it essentially as a transform. So If you really want to be careful here, then yes assume weight could be negative. ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Wed, 10 Mar 2021 18:48:50 GMT, Phil Race <[hidden email]> wrote:
>> Hi, I am not sure about the weight > 0 check ; weight is initialized with 0: weight = 0; and later some values are potentially added up to weight: weight += gi.weight; >> I am not sure about those gi.weight values, maybe they can be negative too ? > > Nothing throws an exception or otherwise prevent this being negative but that would be a weird usage. Typically the weight is either zero or based on the font size .. which ought not to be negative but I don't think anything prevents it and I think we would treat it essentially as a transform. So If you really want to be careful here, then yes assume weight could be negative. By that same logic, then shouldn't absorbweight also be checked as != 0 instead of > 0 as that also uses += gi.weight ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Thu, 11 Mar 2021 04:24:26 GMT, Prasanta Sadhukhan <[hidden email]> wrote:
>> Nothing throws an exception or otherwise prevent this being negative but that would be a weird usage. Typically the weight is either zero or based on the font size .. which ought not to be negative but I don't think anything prevents it and I think we would treat it essentially as a transform. So If you really want to be careful here, then yes assume weight could be negative. > > By that same logic, then shouldn't absorbweight also be checked as != 0 instead of > 0 as that also uses += gi.weight Hi, I am not sure about the absorbweight check; but currently the calculated value weightedAbsorb is only used when absorbing is true. And there the > 0 check is present too boolean absorbing = hitLimit && absorbweight > 0; ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
On Thu, 11 Mar 2021 06:45:05 GMT, Matthias Baesken <[hidden email]> wrote:
>> By that same logic, then shouldn't absorbweight also be checked as != 0 instead of > 0 as that also uses += gi.weight > > Hi, I am not sure about the absorbweight check; but currently the calculated value weightedAbsorb is only used when absorbing is true. And there the > 0 check is present too > > boolean absorbing = hitLimit && absorbweight > 0; I meant that since we are dividing by weight and absorbweight ` weightedDelta = delta / weight;` and weightedAbsorb = (delta - gslimit) / absorbweight; where both weight and absorbweight uses += gi.weight values so if one is checking for >0 then to be consistent, in my opinion, other one should be same or absorbweight also probably should check !=0. ------------- PR: https://git.openjdk.java.net/jdk/pull/2912 |
Free forum by Nabble | Edit this page |