[OpenJDK 2D-Dev] Integrated: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify
On Wed, 10 Mar 2021 12:31:31 GMT, Matthias Baesken 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=AXcqMwpm8sPJZZzONu1k=false=CRITICAL=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. This pull request has now been integrated. Changeset: ea30bd66 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/ea30bd6684fa3003889062a129a5aee1bc9b0024 Stats: 6 lines in 1 file changed: 3 ins; 0 del; 3 mod 8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify Reviewed-by: psadhukhan - PR: https://git.openjdk.java.net/jdk/pull/2912
Re: [OpenJDK 2D-Dev] RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify [v2]
> 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=AXcqMwpm8sPJZZzONu1k=false=CRITICAL=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. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust absorbweight check - Changes: - all: https://git.openjdk.java.net/jdk/pull/2912/files - new: https://git.openjdk.java.net/jdk/pull/2912/files/6b31126f..4e21791e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2912=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2912=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
Re: [OpenJDK 2D-Dev] RFR: 8266545: 8261169 broke Harfbuzz build with gcc 7 and 8
On Wed, 5 May 2021 07:54:20 GMT, Thomas Stuefe wrote: > Harfbuzz upgrade broke Linux x64 build on older gccs. For details see JBS > issue. > > I fixed the issue in the harfbuzz sources, but I am not sure of the policy > here. Do we modify the harfbuzz sources or leave them untouched? Advice is > welcome. > > The patch fixes linux x64 fastdebug and opt build for me. Looks good to me, thanks Thomas ! - Marked as reviewed by mbaesken (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3873
Re: [OpenJDK 2D-Dev] RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify
On Thu, 11 Mar 2021 10:13:31 GMT, Prasanta Sadhukhan wrote: >> 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. should we then better check for if (weight > 0) instead of if (weight != 0) like it is done for absorbweight ? (If you think consistency is important here) - PR: https://git.openjdk.java.net/jdk/pull/2912
Re: [OpenJDK 2D-Dev] RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify
On Thu, 11 Mar 2021 04:24:26 GMT, Prasanta Sadhukhan 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
Re: [OpenJDK 2D-Dev] RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify
On Wed, 10 Mar 2021 12:41:05 GMT, Prasanta Sadhukhan
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=AXcqMwpm8sPJZZzONu1k=false=CRITICAL=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
[OpenJDK 2D-Dev] RFR: JDK-8263362: Avoid division by 0 in java/awt/font/TextJustifier.java justify
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=AXcqMwpm8sPJZZzONu1k=false=CRITICAL=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=2912=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
[OpenJDK 2D-Dev] Integrated: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
On Wed, 27 Jan 2021 08:39:33 GMT, Matthias Baesken wrote: > The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2 > times malloc ; however the memory is not always freed correctly in case of > errors. > See also the related sonar issue : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG This pull request has now been integrated. Changeset: 3aabbd72 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/3aabbd72 Stats: 12 lines in 1 file changed: 8 ins; 0 del; 4 mod 8260432: allocateSpaceForGP in freetypeScaler.c might leak memory Reviewed-by: shade, stuefe - PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory [v3]
> The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2 > times malloc ; however the memory is not always freed correctly in case of > errors. > See also the related sonar issue : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: JDK-8260426 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2250/files - new: https://git.openjdk.java.net/jdk/pull/2250/files/afda19e5..70c3e879 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2250=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2250=01-02 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2250/head:pull/2250 PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory [v2]
On Wed, 27 Jan 2021 12:16:47 GMT, Thomas Stuefe wrote: >> Yes, I think that is better: `realloc` can fail as well, and we don't want >> to leak those? > > Please also reset the free'd pointers to NULL in GPData* before returning. Hello Thomas and Aleksey, I moved the free calls and added the NULL settings. Are you fine with the latest commit? Thanks, Matthias - PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory [v2]
> The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2 > times malloc ; however the memory is not always freed correctly in case of > errors. > See also the related sonar issue : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: JDK-8260426 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2250/files - new: https://git.openjdk.java.net/jdk/pull/2250/files/5c867221..afda19e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2250=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2250=00-01 Stats: 7 lines in 1 file changed: 2 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2250/head:pull/2250 PR: https://git.openjdk.java.net/jdk/pull/2250
Re: [OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
On Wed, 27 Jan 2021 11:41:48 GMT, Aleksey Shipilev wrote:
>> The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2
>> times malloc ; however the memory is not always freed correctly in case of
>> errors.
>> See also the related sonar issue :
>> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG
>
> src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 1290:
>
>> 1288:
>> 1289: /* failure if any of mallocs failed */
>> 1290: if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL)
>
> I think it would be cleaner to free the remaining allocations on the failing
> path:
>
> if (gpdata->pointTypes == NULL || gpdata->pointCoords == NULL) {
>if (gpdata->pointTypes != NULL) free(gpdata->pointTypes);
>if (gpdata->pointCoords != NULL) free(gpdata->pointCoords);
>return 0;
> } else {
>return 1;
> }
Then we would free as well for the realloc code path
} else {
/* do we have enough space? */
...
}
Is this okay?
-
PR: https://git.openjdk.java.net/jdk/pull/2250
[OpenJDK 2D-Dev] RFR: JDK-8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
The function AllocateSpaceForGP in freetypeScaler.c calls potentially 2 times malloc ; however the memory is not always freed correctly in case of errors. See also the related sonar issue : https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B_SBBG2CXpcngxr=false=BLOCKER=BUG - Commit messages: - JDK-8260432 Changes: https://git.openjdk.java.net/jdk/pull/2250/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2250=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260432 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2250/head:pull/2250 PR: https://git.openjdk.java.net/jdk/pull/2250
[OpenJDK 2D-Dev] Integrated: JDK-8258484: AIX build fails in Harfbuzz with XLC 16.01.0000.0006
On Thu, 7 Jan 2021 08:51:25 GMT, Matthias Baesken wrote: > Hello, for a while the AIX build fails with the following error in the > harfbuzz build > 1500-004: (U) INTERNAL COMPILER ERROR while compiling > OT::MarkBasePosFormat1::collect_variation_indices(hb_collect_variation_indices_context_t > *) const. > Compilation ended. Contact your Service Representative and provide the > following information: GRARNN: gr29643 is used before it is defined. > > This xlc 16 version is used for compiling > bash-4.4$ xlc -qversion > IBM XL C/C++ for AIX, V16.1.0 (5725-C72, 5765-J12) > Version: 16.01..0006 > > Solution by IBM compiler support is to use "-qdebug=necan", this > effectively turns off an optimisation called "Early Re-canonizing" on the > harfbuzz lib. This pull request has now been integrated. Changeset: 3f9f86f0 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/3f9f86f0 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8258484: AIX build fails in Harfbuzz with XLC 16.01..0006 Reviewed-by: clanger, mdoerr - PR: https://git.openjdk.java.net/jdk/pull/1972
Re: [OpenJDK 2D-Dev] RFR: JDK-8258484: AIX build fails in Harfbuzz with XLC 16.01.0000.0006 [v2]
> Hello, for a while the AIX build fails with the following error in the > harfbuzz build > 1500-004: (U) INTERNAL COMPILER ERROR while compiling > OT::MarkBasePosFormat1::collect_variation_indices(hb_collect_variation_indices_context_t > *) const. > Compilation ended. Contact your Service Representative and provide the > following information: GRARNN: gr29643 is used before it is defined. > > This xlc 16 version is used for compiling > bash-4.4$ xlc -qversion > IBM XL C/C++ for AIX, V16.1.0 (5725-C72, 5765-J12) > Version: 16.01..0006 > > Solution by IBM compiler support is to use "-qdebug=necan", this > effectively turns off an optimisation called "Early Re-canonizing" on the > harfbuzz lib. Matthias Baesken has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision: - Adjust comment - JDK-8258484 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1972/files - new: https://git.openjdk.java.net/jdk/pull/1972/files/ecd93d06..27205b26 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1972=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1972=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1972.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1972/head:pull/1972 PR: https://git.openjdk.java.net/jdk/pull/1972
[OpenJDK 2D-Dev] RFR: JDK-8258484: AIX build fails in Harfbuzz with XLC 16.01.0000.0006
Hello, for a while the AIX build fails with the following error in the harfbuzz build 1500-004: (U) INTERNAL COMPILER ERROR while compiling OT::MarkBasePosFormat1::collect_variation_indices(hb_collect_variation_indices_context_t *) const. Compilation ended. Contact your Service Representative and provide the following information: GRARNN: gr29643 is used before it is defined. This xlc 16 version is used for compiling bash-4.4$ xlc -qversion IBM XL C/C++ for AIX, V16.1.0 (5725-C72, 5765-J12) Version: 16.01..0006 Solution by IBM compiler support is to use "-qdebug=necan", this effectively turns off an optimisation called "Early Re-canonizing" on the harfbuzz lib. - Commit messages: - JDK-8258484 Changes: https://git.openjdk.java.net/jdk/pull/1972/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1972=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258484 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1972.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1972/head:pull/1972 PR: https://git.openjdk.java.net/jdk/pull/1972
Re: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Tue, 29 Sep 2020 07:11:34 GMT, Matthias Baesken wrote:
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>>
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ?
>>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>>
>> This looks like a plain old bug fix, nothing really to do with the compiler
>> upgrade. The new compiler presumably just
>> has a new warning that brought attention to the problem. As such, I don't
>> think it belongs in a PR that's about issues
>> related to the compiler upgrade. Someone might want to backport just this
>> fix, for example.
>
> Hello Kim, Paul - so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because
> it might be a general problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should I
> open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?
I created
https://bugs.openjdk.java.net/browse/JDK-8253791
8253791: Issue with useAppleColor check in CSystemColors.m
for this issue (Kim and Paul are fine to have a separate JBS issue for this)
-
PR: https://git.openjdk.java.net/jdk/pull/348
Re: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes wrote:
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>
>> Thanks,
>> Paul
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>
>> 2849: if (buf != NULL) {
>> 2850: CodeBuffer buffer(buf);
>> 2851: short locs_buf[80];
>
> This code is just weird. Why is the locs_buf array not declared to be the
> desired type? If the compiler rejects double
> because it isn't relocInfo* then why does it accept short? And if it accepts
> short will it accept int or long long or
> int64_t? Using int64_t would be a clearer change. Though semantically this
> code is awful. :( Should it be intptr_t ?
Currently we have in the existing code various kind of buffers passed into
initialize_shared_locs that compile nicely
on all supported compilers and on Xcode 12 as well ,see
c1_Compilation.cpp
326 char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
327 code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
sharedRuntime.cpp
2681 CodeBuffer buffer(buf);
2682 short buffer_locs[20];
2683 buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
2684
sizeof(buffer_locs)/sizeof(relocInfo));
So probably using short would be consistent to what we do already in the coding
at another place (looking into
relocInfo it would be probably better to use unsigned short or to typedef
unsigned short in the relocInfo class
and use the typedef).
-
PR: https://git.openjdk.java.net/jdk/pull/348
Re: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett wrote:
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>
>> Thanks,
>> Paul
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>
>> 127: NSColor* result = nil;
>> 128:
>> 129: if (colorIndex < ((useAppleColor) ?
>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>> java_awt_SystemColor_NUM_COLORS)) {
>
> This looks like a plain old bug fix, nothing really to do with the compiler
> upgrade. The new compiler presumably just
> has a new warning that brought attention to the problem. As such, I don't
> think it belongs in a PR that's about issues
> related to the compiler upgrade. Someone might want to backport just this
> fix, for example.
Hello Kim, Paul - so should we open a separate bug for the
src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because
it might be a general problem just
uncovered by the new compiler ? Paul , do you want to do this ? Or should I
open a bug and post a separate change for
the useAppleColor check in CSystemColors.m ?
-
PR: https://git.openjdk.java.net/jdk/pull/348
