[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8204211: windows : handle potential C++ exception in GDIRenderer -was
From: "Baesken, Matthias" <matthias.baesken () sap ! com>
Date: 2018-06-06 11:45:51
Message-ID: dafd455344c2484aa541a523975181e6 () sap ! com
[Download RAW message or body]
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@oracle.com]
> Sent: Dienstag, 5. Juni 2018 23:41
> To: Sergey Bylokhov <Sergey.Bylokhov@oracle.com>; Baesken, Matthias
> <matthias.baesken@sap.com>; Langer, Christoph
> <christoph.langer@sap.com>; Thomas Stüfe <thomas.stuefe@gmail.com>;
> 'build-dev@openjdk.java.net' <build-dev@openjdk.java.net>; awt-
> dev@openjdk.java.net
> Cc: 2d-dev <2d-dev@openjdk.java.net>
> Subject: Re: RFR: JDK-8204211: windows : handle potential C++ exception in
> GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
>
>
> 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@sap.com>; Thomas Stüfe
> > > > > > <thomas.stuefe@gmail.com>; 'build-dev@openjdk.java.net' <build-
> > > > > > dev@openjdk.java.net>; awt-dev@openjdk.java.net
> > > > > > Cc: 2d-dev <2d-dev@openjdk.java.net>
> > > > > > Subject: RE: RFR: JDK-8204211: windows : handle potential C++
> > > > > > exception in
> > > > > > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > >
> > > > > > 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@gmail.com>; 'build-
> > > > > > > dev@openjdk.java.net' <build-dev@openjdk.java.net>; awt-
> > > > > > > dev@openjdk.java.net
> > > > > > > Cc: 2d-dev <2d-dev@openjdk.java.net>; Langer, Christoph
> > > > > > > <christoph.langer@sap.com>
> > > > > > > Subject: RE: RFR: JDK-8204211: windows : handle potential C++
> > > > > > > exception in
> > > > > > > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> > > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > > >
> > > > > > > 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@gmail.com>
> > > > > > > > Cc: '2d-dev' <2d-dev@openjdk.java.net>; Langer, Christoph
> > > > > > > > <christoph.langer@sap.com>
> > > > > > > > Subject: RE: RFR: JDK-8204211: windows : handle potential C++
> > > > > > > > exception
> > > > > > in
> > > > > > > > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> > > > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > > > >
> > > > > > > > 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:18:03 2018
> > > > > > > > +0200
> > > > > > > > @@ -213,6 +213,7 @@
> > > > > > > > LIBAWT_CFLAGS += -fgcse-after-reload
> > > > > > > > endif
> > > > > > > >
> > > > > > > >
> > > > > > > > $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \
> > > > > > > > NAME := awt, \
> > > > > > > > SRC := $(LIBAWT_DIRS), \
> > > > > > > > @@ -224,7 +225,7 @@
> > > > > > > > format-nonliteral parentheses, \
> > > > > > > > DISABLED_WARNINGS_clang := logical-op-parentheses extern-
> > > > > > initializer,
> > > > > > > \
> > > > > > > > DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE, \
> > > > > > > > - DISABLED_WARNINGS_microsoft := 4297 4244 4267 4996, \
> > > > > > > > + DISABLED_WARNINGS_microsoft := 4244 4267 4996, \
> > > > > > > > ASFLAGS := $(LIBAWT_ASFLAGS), \
> > > > > > > > LDFLAGS := $(LDFLAGS_JDKLIB) $(call
> > > > > > > > SET_SHARED_LIBRARY_ORIGIN),
> > > > > > \
> > > > > > > > LDFLAGS_macosx := -L$(INSTALL_LIBRARIES_HERE), \
> > > > > > > > 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:18:03 2018 +0200
> > > > > > > > @@ -85,7 +85,13 @@
> > > > > > > > *pNpoints = outpoints;
> > > > > > > > }
> > > > > > > > if (outpoints > POLYTEMPSIZE) {
> > > > > > > > - pPoints = (POINT *) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> > > > > > > > sizeof(POINT), outpoints);
> > > > > > > > + try {
> > > > > > > > + pPoints = (POINT *) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> > > > > > > > sizeof(POINT), outpoints);
> > > > > > > > + } catch (const std::bad_alloc&) {
> > > > > > > > + 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:18:03 2018 +0200
> > > > > > > > @@ -873,7 +873,12 @@
> > > > > > > > int numSizes = ::DeviceCapabilities(printerName,
> > > > > > > > printerPort,
> > > > > > > > DC_PAPERS, NULL, NULL);
> > > > > > > > if (numSizes > 0) {
> > > > > > > > - LPTSTR papers =
> > > > > > > > (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> > > > > > > > numSizes, sizeof(WORD));
> > > > > > > > + LPTSTR papers;
> > > > > > > > + try {
> > > > > > > > + papers = (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> > > > > > > numSizes,
> > > > > > > > sizeof(WORD));
> > > > > > > > + } catch (const std::bad_alloc&) {
> > > > > > > > + papers = NULL;
> > > > > > > > + }
> > > > > > > > if (papers != NULL &&
> > > > > > > > > > DeviceCapabilities(printerName, printerPort,
> > > > > > > > DC_PAPERS, papers, NULL) !=
> > > > > > > > -1) {
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Baesken, Matthias
> > > > > > > > > Sent: Freitag, 1. Juni 2018 14:18
> > > > > > > > > To: 'Thomas Stüfe' <thomas.stuefe@gmail.com>
> > > > > > > > > Cc: 2d-dev <2d-dev@openjdk.java.net>; Langer, Christoph
> > > > > > > > > <christoph.langer@sap.com>
> > > > > > > > > Subject: RE: RFR: JDK-8204211: windows : handle potential C++
> > > > > > exception
> > > > > > > in
> > > > > > > > > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> > > > > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > > > > >
> > > > > > > > > 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::bad_alloc& (non-const) is caught .
> > > > > > > > >
> > > > > > > > > For example :
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > alloc.h 170 } catch (std::bad_alloc&) { \
> > > > > > > > > 177 } catch (std::bad_alloc&) { \
> > > > > > > > > 200 } catch (std::bad_alloc&) { \
> > > > > > > > > 206 } catch (std::bad_alloc&) { \
> > > > > > > > >
> > > > > > > > > awt_InputTextInfor.cpp 223 } catch (std::bad_alloc&) {
> > > > > > > > > 247 } catch (std::bad_alloc&) {
> > > > > > > > > 317 } catch (std::bad_alloc&) {
> > > > > > > > > 372 } catch (std::bad_alloc&) {
> > > > > > > > > 407 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > awt_DnDDT.cpp 203 } catch (std::bad_alloc&) {
> > > > > > > > > 264 } catch (std::bad_alloc&) {
> > > > > > > > > 305 } catch (std::bad_alloc&) {
> > > > > > > > > 366 } catch (std::bad_alloc&) {
> > > > > > > > > 582 } catch (std::bad_alloc&) {
> > > > > > > > > 635 } catch (std::bad_alloc&) {
> > > > > > > > > 653 } catch (std::bad_alloc&) {
> > > > > > > > > 698 } catch (std::bad_alloc&) {
> > > > > > > > > 739 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > awt_Desktop.cpp 148 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > WPrinterJob.cpp 166 } catch (std::bad_alloc&) {
> > > > > > > > > 345 } catch (std::bad_alloc&) {
> > > > > > > > > 425 } catch (std::bad_alloc&) {
> > > > > > > > > 488 } catch (std::bad_alloc&) {
> > > > > > > > > 631 } catch (std::bad_alloc&) {
> > > > > > > > > 709 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > awt_ole.h 158 } catch (std::bad_alloc&) {\
> > > > > > > > >
> > > > > > > > > awt_DesktopProperties.cpp 125 catch (std::bad_alloc&) {
> > > > > > > > > 269 catch (std::bad_alloc&) {
> > > > > > > > > 647 catch (std::bad_alloc&) {
> > > > > > > > > 664 catch (std::bad_alloc&) {
> > > > > > > > > 689 catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > awt_PrintDialog.cpp 225 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > awt_DataTransferer.cpp 310 } catch (std::bad_alloc&) {
> > > > > > > > > 724 } catch (std::bad_alloc &) {
> > > > > > > > > 792 } catch (std::bad_alloc &) {
> > > > > > > > >
> > > > > > > > > awt_MenuItem.cpp 328 } catch (std::bad_alloc&) {
> > > > > > > > > 348 } catch (std::bad_alloc&) {
> > > > > > > > > 524 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > ShellFolder2.cpp 1410 } catch (std::bad_alloc&) {
> > > > > > > > > 1435 } catch (std::bad_alloc&) {
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > Best regards, Matthias
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Thomas Stüfe [mailto:thomas.stuefe@gmail.com]
> > > > > > > > > > Sent: Freitag, 1. Juni 2018 12:02
> > > > > > > > > > To: Baesken, Matthias <matthias.baesken@sap.com>
> > > > > > > > > > Cc: 2d-dev <2d-dev@openjdk.java.net>; Langer, Christoph
> > > > > > > > > > <christoph.langer@sap.com>
> > > > > > > > > > Subject: Re: RFR: JDK-8204211: windows : handle potential C++
> > > > > > > exception
> > > > > > > > in
> > > > > > > > > > GDIRenderer -was : RE: [OpenJDK 2D-Dev] java2d coding using
> > > > > > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > > > > > >
> > > > > > > > > > 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::bad_alloc&) {}
> > > > > > > > > >
> > > > > > > > > > Fine otherwise. I do not need another webrev.
> > > > > > > > > >
> > > > > > > > > > Best Regards, Thomas
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Fri, Jun 1, 2018 at 10:39 AM, Baesken, Matthias
> > > > > > > > > > <matthias.baesken@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@gmail.com]
> > > > > > > > > > > > Sent: Mittwoch, 30. Mai 2018 17:37
> > > > > > > > > > > > To: Baesken, Matthias <matthias.baesken@sap.com>
> > > > > > > > > > > > Cc: 2d-dev <2d-dev@openjdk.java.net>
> > > > > > > > > > > > Subject: Re: [OpenJDK 2D-Dev] java2d coding using
> > > > > > > > > > > > SAFE_SIZE_ARRAY_ALLOC / safe_Malloc
> > > > > > > > > > > >
> > > > > > > > > > > > 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 bad_alloc, just to be
> > > > > > > > > > > > safe).
> > > > > > > > > > > >
> > > > > > > > > > > > Best Regards, Thomas
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, May 30, 2018 at 5:08 PM, Baesken, Matthias
> > > > > > > > > > > > <matthias.baesken@sap.com> wrote:
> > > > > > > > > > > > > Hello , there is still some java2d coding where
> > > > > > > > > > SAFE_SIZE_ARRAY_ALLOC /
> > > > > > > > > > > > > safe_Malloc 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 *)
> > > > > > > > > > > > > SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> > > > > > > > > > > > > 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
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > >
> > >
> >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic