RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify

[Matthias Baesken]

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

[Prasanta Sadhukhan-2]

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

[Matthias Baesken]

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

[Phil Race]

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

[Prasanta Sadhukhan-2]

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

[Matthias Baesken]

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

[Prasanta Sadhukhan-2]

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