RFR: JDK-8204211: windows : handle potential C++ exception in GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using SAFE_SIZE_ARRAY_ALLOC (original) (raw)

Baesken, Matthias matthias.baesken at sap.com
Wed Jun 6 11:45:51 UTC 2018

Hi Sergey, thanks for checking . @Christoph : copyright years updated in the cpp files . @Phil , I checked the indentation it looked indeed strange in the udiff however in the cpp file itself it looks ok to me .

Thanks for the reviews and best regards, Matthias

-----Original Message----- From: Phil Race [mailto:philip.race at oracle.com] Sent: Dienstag, 5. Juni 2018 23:41 To: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>; Baesken, Matthias <matthias.baesken at sap.com>; Langer, Christoph <christoph.langer at sap.com>; Thomas Stüfe <thomas.stuefe at gmail.com>; 'build-dev at openjdk.java.net' <build-dev at openjdk.java.net>; awt- dev at openjdk.java.net Cc: 2d-dev <2d-dev at openjdk.java.net> Subject: Re: RFR: JDK-8204211: windows : handle potential C++ exception in GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using SAFESIZEARRAYALLOC / safeMalloc

In that case Matthias is good to go after fixing the indentation. -phil. On 06/05/2018 01:37 PM, Sergey Bylokhov wrote: > I have checked the fix using mach5. > > On 05/06/2018 12:45, Phil Race wrote: >> Oh .. can I please ask that you make sure that VS2017 is OK with the >> re-enabled >> warning ? I seriously doubt that it has anything new to add over >> VS2013, but a jdk-submit >> will tell you if it has .. >> >> VS2017 is now the default so a jdk-submit will use that. >> >> -phil. >> >> On 06/05/2018 12:43 PM, Phil Race wrote: >>> This looks good to me except for what looks like in my browser like >>> missing indentation in >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8204211.1/src/java.deskto p/windows/native/libawt/java2d/windows >>> >>> /GDIRenderer.cpp.udiff.html >>> >>> You can fix that with or without an updated webrev. >>> >>> Good to clear a class of warnings. >>> >>> -phil. >>> >>> On 06/05/2018 12:47 AM, Baesken, Matthias wrote: >>>> Hi Christoph, thank's for the review . >>>> Could I have a second one for example from the awt or build-dev >>>> reviewers ? >>>> >>>> Best Regards, Matthias >>>> >>>> >>>>> -----Original Message----- >>>>> From: Langer, Christoph >>>>> Sent: Montag, 4. Juni 2018 16:49 >>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; Thomas Stüfe >>>>> <thomas.stuefe at gmail.com>; 'build-dev at openjdk.java.net' >>>> dev at openjdk.java.net>; awt-dev at openjdk.java.net >>>>> Cc: 2d-dev <2d-dev at openjdk.java.net> >>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ >>>>> exception in >>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using >>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>> >>>>> Hi Matthias, >>>>> >>>>> looks good to me. >>>>> >>>>> Don't forget the Copyright years. >>>>> >>>>> Best regards >>>>> Christoph >>>>> >>>>>> -----Original Message----- >>>>>> From: Baesken, Matthias >>>>>> Sent: Montag, 4. Juni 2018 16:20 >>>>>> To: Thomas Stüfe <thomas.stuefe at gmail.com>; 'build- >>>>>> dev at openjdk.java.net' <build-dev at openjdk.java.net>; awt- >>>>>> dev at openjdk.java.net >>>>>> Cc: 2d-dev <2d-dev at openjdk.java.net>; Langer, Christoph >>>>>> <christoph.langer at sap.com> >>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ >>>>>> exception in >>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using >>>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>>> >>>>>> Hello, I prepared a second webrev. >>>>>> >>>>>> - use now const-reference in the catch-statements as suggested by >>>>> Thomas >>>>>> - reenabled the cl warning showing the exception issues in >>>>>> make/lib/Awt2dLibraries.gmk >>>>>> - found a second place in WPrinterJob.cpp with similar issues >>>>>> after >>>>>> reenabling the warnings >>>>>> >>>>>> Please review : >>>>>> >>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8204211.1/ >>>>>> >>>>>> (cc-ing build-dev because of the makefile change, and >>>>>> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp >>>>>> because of the awt change ) >>>>>> >>>>>> >>>>>> Thanks, Matthias >>>>>> >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Baesken, Matthias >>>>>>> Sent: Montag, 4. Juni 2018 09:24 >>>>>>> To: 'Thomas Stüfe' <thomas.stuefe at gmail.com> >>>>>>> Cc: '2d-dev' <2d-dev at openjdk.java.net>; Langer, Christoph >>>>>>> <christoph.langer at sap.com> >>>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ >>>>>>> exception >>>>> in >>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using >>>>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>>>> >>>>>>> A small update - I found a second place in WPrinterJob.cpp >>>>>>> where the >>>>>>> exception handling is missing. After fixing both places I can >>>>>>> reenable >>>>>>> warning 4297 in >>>>>>> Awt2dLibraries.gmk which has been disabled before , probably >>>>> because >>>>>> of >>>>>>> the warnings generated when the exceptions where not handled . >>>>>>> Should I update the change with the other file + makefile change ? >>>>>>> >>>>>>> Best regards, Matthias >>>>>>> >>>>>>> >>>>>>>> hg diff >>>>>>> diff -r 12fe57c319e1 make/lib/Awt2dLibraries.gmk >>>>>>> --- a/make/lib/Awt2dLibraries.gmk Tue Apr 10 11:02:09 2018 >>>>>>> +0800 >>>>>>> +++ b/make/lib/Awt2dLibraries.gmk Mon Jun 04 09🔞03 2018 >>>>>>> +0200 >>>>>>> @@ -213,6 +213,7 @@ >>>>>>> LIBAWTCFLAGS += -fgcse-after-reload >>>>>>> endif >>>>>>> >>>>>>> _>>>>>>> (eval(eval (eval(call SetupJdkLibrary, BUILDLIBAWT, _ _>>>>>>> NAME := awt, _ _>>>>>>> SRC := $(LIBAWTDIRS), _ >>>>>>> @@ -224,7 +225,7 @@ _>>>>>>> format-nonliteral parentheses, _ >>>>>>> DISABLEDWARNINGSclang := logical-op-parentheses extern- >>>>> initializer, _>>>>>> _ _>>>>>>> DISABLEDWARNINGSsolstudio := EDECLARATIONINCODE, _ _>>>>>>> - DISABLEDWARNINGSmicrosoft := 4297 4244 4267 4996, _ _>>>>>>> + DISABLEDWARNINGSmicrosoft := 4244 4267 4996, _ _>>>>>>> ASFLAGS := $(LIBAWTASFLAGS), _ >>>>>>> LDFLAGS := (LDFLAGSJDKLIB)(LDFLAGSJDKLIB) (LDFLAGSJDKLIB)(call >>>>>>> SETSHAREDLIBRARYORIGIN), _>>>>> _ _>>>>>>> LDFLAGSmacosx := -L$(INSTALLLIBRARIESHERE), _ >>>>>>> diff -r 12fe57c319e1 >>>>>>> >>>>> src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp >>>>>>> --- >>>>>>> >>>>> a/src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.c >>>>>>> pp Tue Apr 10 11:02:09 2018 +0800 >>>>>>> +++ >>>>>>> >>>>> b/src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.c >>>>>>> pp Mon Jun 04 09🔞03 2018 +0200 >>>>>>> @@ -85,7 +85,13 @@ >>>>>>> *pNpoints = outpoints; >>>>>>> } >>>>>>> if (outpoints > POLYTEMPSIZE) { >>>>>>> - pPoints = (POINT *) SAFESIZEARRAYALLOC(safeMalloc, >>>>>>> sizeof(POINT), outpoints); >>>>>>> + try { >>>>>>> + pPoints = (POINT *) SAFESIZEARRAYALLOC(safeMalloc, >>>>>>> sizeof(POINT), outpoints); >>>>>>> + } catch (const std::badalloc&) { >>>>>>> + return NULL; >>>>>>> + } >>>>>>> } >>>>>>> BOOL isempty = fixend; >>>>>>> for (int i = 0; i < npoints; i++) {_ _>>>>>>> diff -r 12fe57c319e1 >>>>>>> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp >>>>>>> --- >>>>> a/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp >>>>>>> Tue Apr 10 11:02:09 2018 +0800 >>>>>>> +++ >>>>>> b/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp >>>>>>> Mon Jun 04 09🔞03 2018 +0200 >>>>>>> @@ -873,7 +873,12 @@ >>>>>>> int numSizes = ::DeviceCapabilities(printerName, >>>>>>> printerPort, >>>>>>> DCPAPERS, NULL, NULL); >>>>>>> if (numSizes > 0) { >>>>>>> - LPTSTR papers = >>>>>>> (LPTSTR)SAFESIZEARRAYALLOC(safeMalloc, >>>>>>> numSizes, sizeof(WORD)); >>>>>>> + LPTSTR papers; >>>>>>> + try { >>>>>>> + papers = (LPTSTR)SAFESIZEARRAYALLOC(safeMalloc, >>>>>> numSizes, >>>>>>> sizeof(WORD)); >>>>>>> + } catch (const std::badalloc&) { >>>>>>> + papers = NULL; >>>>>>> + } >>>>>>> if (papers != NULL && >>>>>>> ::DeviceCapabilities(printerName, printerPort, >>>>>>> DCPAPERS, papers, NULL) != >>>>>>> -1) { >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Baesken, Matthias >>>>>>>> Sent: Freitag, 1. Juni 2018 14:18 >>>>>>>> To: 'Thomas Stüfe' <thomas.stuefe at gmail.com> >>>>>>>> Cc: 2d-dev <2d-dev at openjdk.java.net>; Langer, Christoph >>>>>>>> <christoph.langer at sap.com> >>>>>>>> Subject: RE: RFR: JDK-8204211: windows : handle potential C++ >>>>> exception >>>>>> in >>>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using >>>>>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>>>>> >>>>>>>> Hi Thomas , using the const-reference sounds like a good >>>>>>>> idea ( I just >>>>>>>> copied from other locations in the source code where (almost?) >>>>> always >>>>>>>> std::badalloc& (non-const) is caught . >>>>>>>> >>>>>>>> For example : >>>>>>>> >>>>>>>> _>>>>>>>> alloc.h 170 } catch (std::badalloc&) { _ _>>>>>>>> 177 } catch (std::badalloc&) { _ _>>>>>>>> 200 } catch (std::badalloc&) { _ _>>>>>>>> 206 } catch (std::badalloc&) { _ >>>>>>>> >>>>>>>> awtInputTextInfor.cpp 223 } catch (std::badalloc&) { >>>>>>>> 247 } catch (std::badalloc&) { >>>>>>>> 317 } catch (std::badalloc&) { >>>>>>>> 372 } catch (std::badalloc&) { >>>>>>>> 407 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> awtDnDDT.cpp 203 } catch (std::badalloc&) { >>>>>>>> 264 } catch (std::badalloc&) { >>>>>>>> 305 } catch (std::badalloc&) { >>>>>>>> 366 } catch (std::badalloc&) { >>>>>>>> 582 } catch (std::badalloc&) { >>>>>>>> 635 } catch (std::badalloc&) { >>>>>>>> 653 } catch (std::badalloc&) { >>>>>>>> 698 } catch (std::badalloc&) { >>>>>>>> 739 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> awtDesktop.cpp 148 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> WPrinterJob.cpp 166 } catch (std::badalloc&) { >>>>>>>> 345 } catch (std::badalloc&) { >>>>>>>> 425 } catch (std::badalloc&) { >>>>>>>> 488 } catch (std::badalloc&) { >>>>>>>> 631 } catch (std::badalloc&) { >>>>>>>> 709 } catch (std::badalloc&) { >>>>>>>> _>>>>>>>> awtole.h 158 } catch (std::badalloc&) {_ >>>>>>>> >>>>>>>> awtDesktopProperties.cpp 125 catch (std::badalloc&) { >>>>>>>> 269 catch (std::badalloc&) { >>>>>>>> 647 catch (std::badalloc&) { >>>>>>>> 664 catch (std::badalloc&) { >>>>>>>> 689 catch (std::badalloc&) { >>>>>>>> >>>>>>>> awtPrintDialog.cpp 225 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> awtDataTransferer.cpp 310 } catch (std::badalloc&) { >>>>>>>> 724 } catch (std::badalloc &) { >>>>>>>> 792 } catch (std::badalloc &) { >>>>>>>> >>>>>>>> awtMenuItem.cpp 328 } catch (std::badalloc&) { >>>>>>>> 348 } catch (std::badalloc&) { >>>>>>>> 524 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> ShellFolder2.cpp 1410 } catch (std::badalloc&) { >>>>>>>> 1435 } catch (std::badalloc&) { >>>>>>>> >>>>>>>> ... >>>>>>>> >>>>>>>> Best regards, Matthias >>>>>>>> >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] >>>>>>>>> Sent: Freitag, 1. Juni 2018 12:02 >>>>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com> >>>>>>>>> Cc: 2d-dev <2d-dev at openjdk.java.net>; Langer, Christoph >>>>>>>>> <christoph.langer at sap.com> >>>>>>>>> Subject: Re: RFR: JDK-8204211: windows : handle potential C++ >>>>>> exception >>>>>>> in >>>>>>>>> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using >>>>>>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>>>>>> >>>>>>>>> Hi Matthias, >>>>>>>>> >>>>>>>>> Please consider catching all exceptions, not just std::alloc: >>>>>>>>> >>>>>>>>> } catch (...) { return NULL; } >>>>>>>>> >>>>>>>>> and doing it at the exit extern "C" function, not somewhere >>>>>>>>> internally. Regardless of which exceptions get thrown around >>>>>>>>> below >>>>>> you >>>>>>>>> and by whom, you are safe that way. >>>>>>>>> >>>>>>>>> However, if you want to keep your patch as it is, please catch at >>>>>>>>> least as const reference: >>>>>>>>> >>>>>>>>> } catch (const std::badalloc&) {} >>>>>>>>> >>>>>>>>> Fine otherwise. I do not need another webrev. >>>>>>>>> >>>>>>>>> Best Regards, Thomas >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Jun 1, 2018 at 10:39 AM, Baesken, Matthias >>>>>>>>> <matthias.baesken at sap.com> wrote: >>>>>>>>>> Hi Thomas , thanks for the feedback. >>>>>>>>>> I created a bug and change for the excpetion handling in >>>>>>>> GDIRenderer.cpp >>>>>>>>> . >>>>>>>>>> Please review . >>>>>>>>>> >>>>>>>>>> Thanks, Matthias >>>>>>>>>> >>>>>>>>>> Bug: >>>>>>>>>> >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204211 >>>>>>>>>> >>>>>>>>>> JDK-8204211: windows : handle potential C++ exception in >>>>>> GDIRenderer >>>>>>>>>> >>>>>>>>>> Change : >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8204211/ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> -----Original Message----- >>>>>>>>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] >>>>>>>>>>> Sent: Mittwoch, 30. Mai 2018 17:37 >>>>>>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com> >>>>>>>>>>> Cc: 2d-dev <2d-dev at openjdk.java.net> >>>>>>>>>>> Subject: Re: [OpenJDK 2D-Dev] java2d coding using >>>>>>>>>>> SAFESIZEARRAYALLOC / safeMalloc >>>>>>>>>>> >>>>>>>>>>> Letting c++ exceptions escape from extern "C" functions is >>>>>>>>>>> UB and >>>>>>> may >>>>>>>>>>> (and probably will) crash the process. This should be fixed. >>>>> Approach >>>>>>>>>>> taken by JDK-8039394 is fine (I would probably catch every C++ >>>>>>>>>>> exception with catch(...), not just badalloc, just to be >>>>>>>>>>> safe). >>>>>>>>>>> >>>>>>>>>>> Best Regards, Thomas >>>>>>>>>>> >>>>>>>>>>> On Wed, May 30, 2018 at 5:08 PM, Baesken, Matthias >>>>>>>>>>> <matthias.baesken at sap.com> wrote: >>>>>>>>>>>> Hello , there is still some java2d coding where >>>>>>>>> SAFESIZEARRAYALLOC / >>>>>>>>>>>> safeMalloc is used and the (potentially occurring) >>>>>>>>>>>> exception >>>>> is >>>>>>> not >>>>>>>>>>>> handled . >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> This leads to CL warnings (when enabled ) like >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> " function assumed not to throw an exception but does ; The >>>>>>> function >>>>>>>> is >>>>>>>>>>>> extern "C" and /EHc was specified" >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Example : >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>> java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> static POINT *TransformPoly() >>>>>>>>>>>> >>>>>>>>>>>> ….. >>>>>>>>>>>> >>>>>>>>>>>> if (outpoints > POLYTEMPSIZE) { >>>>>>>>>>>> >>>>>>>>>>>> pPoints = (POINT *) >>>>>>>>>>>> SAFESIZEARRAYALLOC(safeMalloc, >>>>>>>>>>>> sizeof(POINT), outpoints); >>>>>>>>>>>> >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Should we add exception handling here ? >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Similar fixes were done in the change 8039394: Compiler >>>>> warnings >>>>>>>>> about >>>>>>>>>>> C++ >>>>>>>>>>>> exceptions in windows printing code >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8039394 >>>>>>>>>>>> >>>>>>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/823387e2bf42 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Best regards, Matthias >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>> >> > >

More information about the build-dev mailing list