[OpenJDK 2D-Dev] <AWT Dev> RFR(M): 8170525: Fix minor issues in awt coding

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Wed Dec 7 07:20:29 UTC 2016


Hi Phil, 

does that mean I can push the change, or will this happen through jprt?

Best regards,
  Goetz.

> -----Original Message-----
> From: Philip Race [mailto:philip.race at oracle.com]
> Sent: Tuesday, December 06, 2016 4:21 AM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>; awt-
> dev at openjdk.java.net; 2d-dev <2d-dev at openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> RFR(M): 8170525: Fix minor
> issues in awt coding
> 
> I didn't eyeball what you changed but JPRT is now happy.
> The build passes on all platforms...
> 
> -phil.
> 
> On 12/5/16, 2:47 PM, Lindenmaier, Goetz wrote:
> > Hi Phil,
> >
> > sorry for that! I fixed it, there is an other occurrence, too.
> > I double checked all the casts, there was also a mp_flags<->  mp_sign
> wrong in mpi.c
> > http://cr.openjdk.java.net/~goetz/wr16/8170525-awt-dev/webrev.05/
> > (Also rebased the change)
> >
> > Best regards,
> >    Goetz
> >
> >> -----Original Message-----
> >> From: Phil Race [mailto:philip.race at oracle.com]
> >> Sent: Monday, December 05, 2016 7:26 PM
> >> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>; Sergey Bylokhov
> >> <Sergey.Bylokhov at oracle.com>
> >> Cc: awt-dev at openjdk.java.net; 2d-dev<2d-dev at openjdk.java.net>
> >> Subject: Re: [OpenJDK 2D-Dev]<AWT Dev>  RFR(M): 8170525: Fix minor
> >> issues in awt coding
> >>
> >> I tried it .. and just as well I did. It fails in the crypto code on Mac.
> >>
> >> jdk/src/jdk.crypto.ec/share/native/libsunec/impl/ec.c:261:12: error:
> >> expression which evaluates to zero treated as a null pointer constant of
> type
> >> 'mp_digit *' (aka 'unsigned long *') [-Werror,-Wnon-literal-null-conversion]
> >>       k.dp = (mp_digit)0;
> >>              ^~~~~~~~~~~
> >> 1 error generated.
> >>
> >> -phil.
> >>
> >>
> >>
> >> On 12/3/2016 9:53 AM, Lindenmaier, Goetz wrote:
> >>> Hi Phil,
> >>>
> >>> that would be great, unfortunately I don't have access to JPRT.
> >>>
> >>> Thanks,
> >>>     Goetz.
> >>>
> >>>> -----Original Message-----
> >>>> From: Phil Race [mailto:philip.race at oracle.com]
> >>>> Sent: Friday, December 02, 2016 8:46 PM
> >>>> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>; Sergey
> Bylokhov
> >>>> <Sergey.Bylokhov at oracle.com>
> >>>> Cc: awt-dev at openjdk.java.net; 2d-dev<2d-dev at openjdk.java.net>
> >>>> Subject: Re: [OpenJDK 2D-Dev]<AWT Dev>  RFR(M): 8170525: Fix minor
> >>>> issues in awt coding
> >>>>
> >>>> I had no other comments, except that it would be good to be sure
> >>>> that this builds on all relevant platforms .. using the 'blessed' compilers.
> >>>> If you like I can submit a JPRT job for you based on this patch.
> >>>>
> >>>> -phil.
> >>>>
> >>>> On 12/01/2016 11:58 PM, Lindenmaier, Goetz wrote:
> >>>>> Hi Phil,
> >>>>>
> >>>>> I added the initialization to the other function.
> >>>>> http://cr.openjdk.java.net/~goetz/wr16/8170525-awt-
> dev/webrev.04/
> >>>>>
> >>>>> I missed that because Coverity didn't complain.
> >>>>> It's not a perfect tool, but it finds sufficient issues
> >>>>> to make it worthwhile.   Is the other awt code fine?
> >>>>>
> >>>>> Best regards,
> >>>>>      Goetz.
> >>>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Phil Race [mailto:philip.race at oracle.com]
> >>>>>> Sent: Donnerstag, 1. Dezember 2016 22:31
> >>>>>> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>; Sergey
> >> Bylokhov
> >>>>>> <Sergey.Bylokhov at oracle.com>; Vincent Ryan
> >>>> <vincent.x.ryan at oracle.com>
> >>>>>> Cc: awt-dev at openjdk.java.net; 2d-dev<2d-dev at openjdk.java.net>;
> >>>> security-
> >>>>>> dev at openjdk.java.net
> >>>>>> Subject: Re: [OpenJDK 2D-Dev]<AWT Dev>  RFR(M): 8170525: Fix
> minor
> >>>> issues
> >>>>>> in awt coding
> >>>>>>
> >>>>>> Sorry. it is
> >>>>>>         ops->GetRasInfo(env, ops, lockInfo);
> >>>>>> that initialises it ..
> >>>>>>
> >>>>>>
> >>>>>> That is still before the dereference
> >>>>>> Anyway, what was the reason for updating one function, but not the
> >>>> other.
> >>>>>> I don't mind the change, but the inconsistency looks odd.
> >>>>>>
> >>>>>> -phil.
> >>>>>>
> >>>>>> On 12/01/2016 03:03 AM, Lindenmaier, Goetz wrote:
> >>>>>>> Hi Phil,
> >>>>>>>
> >>>>>>>> Did you actually compile this ? The variable is called "rasBase", not
> >>>>>>>> "resBase".
> >>>>>>> Yes, I compiled it, and I fixed the error, but that was another repo
> >>>>>>> I use for the coverity checks.  Somehow it did not find its way into
> >>>>>>> the webrev.
> >>>>>>>
> >>>>>>> I don't see where ops->Lock() initializes this field.
> >>>>>>> ops->GetRasInfo(env, ops, lockInfo); does so if it resolves
> >>>>>>> to BufImg_GetRasInfo().  I can't look in our code scan results
> >>>>>>> because the issue is gone after fixing it, that lists the path
> >>>>>>> of execution that leads to the issue.
> >>>>>>> If you are sure this is correct I will remove the initialization,
> >>>>>>> else I will also put it into the other method.
> >>>>>>>
> >>>>>>> DBN_GetPixelPointer checks lockInfo->rasBase for NULL and
> >>>>>>> does what looks like the error case if it's NULL. Therefore NULL
> >>>>>>> seemed a good initialization to me.
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>>       Goetz.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Phil Race [mailto:philip.race at oracle.com]
> >>>>>>>> Sent: Mittwoch, 30. November 2016 20:59
> >>>>>>>> To: Sergey Bylokhov<Sergey.Bylokhov at oracle.com>; Lindenmaier,
> >>>> Goetz
> >>>>>>>> <goetz.lindenmaier at sap.com>; Vincent Ryan
> >>>> <vincent.x.ryan at oracle.com>
> >>>>>>>> Cc: awt-dev at openjdk.java.net; 2d-dev<2d-
> dev at openjdk.java.net>;
> >>>> security-
> >>>>>>>> dev at openjdk.java.net
> >>>>>>>> Subject: Re: [OpenJDK 2D-Dev]<AWT Dev>  RFR(M): 8170525: Fix
> >> minor
> >>>>>> issues
> >>>>>>>> in awt coding
> >>>>>>>>
> >>>>>>>> Hi Goetz,
> >>>>>>>>
> >>>>>>>>>       DataBufferNative.c
> >>>>>>>>> Using uninitialized value lockInfo.rasBase when calling
> >>>>>> DBN_GetPixelPointer.
> >>>>>>>>        75     lockInfo.resBase = NULL;
> >>>>>>>>
> >>>>>>>> Did you actually compile this ? The variable is called "rasBase", not
> >>>>>>>> "resBase".
> >>>>>>>>
> >>>>>>>> And strictly there is no problem since inside  DBN_GetPixelPointer
> >>>>>>>> the code calls ops->Lock which should initialise this.
> >>>>>>>> A "rasBase" of 0 isn't really any better than a random one ..
> >>>>>>>>
> >>>>>>>> Also I don't see why there's a problem here and not in
> >>>>>>>> the function immediately following since it is the exact same case.
> >>>>>>>>
> >>>>>>>> -phil.
> >>>>>>>>
> >>>>>>>> On 11/30/2016 10:00 AM, Sergey Bylokhov wrote:
> >>>>>>>>> cc 2d-dev.
> >>>>>>>>>
> >>>>>>>>> On 30.11.16 18:41, Lindenmaier, Goetz wrote:
> >>>>>>>>>> Hi Vincent,
> >>>>>>>>>>
> >>>>>>>>>> thanks for the quit review!
> >>>>>>>>>> Good catch that I lost the change to p11_mutex.c ... I had to
> >> change
> >>>>>>>>>> it and it fell out of my patches.
> >>>>>>>>>> I edited the Last Modified Date, and also updated the copyright
> >>>>>>>>>> messages.
> >>>>>>>>>> New webrev:
> >>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170525-awt-dev/
> >>>>>>>>>>
> >>>>>>>>>> Best regards,
> >>>>>>>>>>       Goetz.
> >>>>>>>>>>
> >>>>>>>>>> (Am I correct that your openJdk name is Vinnie?)
> >>>>>>>>>>
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: Vincent Ryan [mailto:vincent.x.ryan at oracle.com]
> >>>>>>>>>>> Sent: Mittwoch, 30. November 2016 14:53
> >>>>>>>>>>> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>
> >>>>>>>>>>> Cc: awt-dev at openjdk.java.net; security-
> dev at openjdk.java.net
> >>>>>>>>>>> Subject: Re: RFR(M): 8170525: Fix minor issues in awt coding
> >>>>>>>>>>>
> >>>>>>>>>>> Hello Goetz,
> >>>>>>>>>>>
> >>>>>>>>>>> Please modify the bug summary to reference ECC too.
> >>>>>>>>>>> Your ECC changes look fine but the ‘Last Modified Date’ line in
> >> the
> >>>>>>>>>>> 4 source
> >>>>>>>>>>> code headers will need to be updated/added.
> >>>>>>>>>>>
> >>>>>>>>>>> BTW p11_mutex.c is listed below but appears to be missing
> from
> >>>> the
> >>>>>>>>>>> webrev.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>         On 30 Nov 2016, at 13:12, Lindenmaier, Goetz
> >>>>>>>>>>> <goetz.lindenmaier at sap.com
> >>>> <mailto:goetz.lindenmaier at sap.com>  >
> >>>>>>>> wrote:
> >>>>>>>>>>>         Hi,
> >>>>>>>>>>>
> >>>>>>>>>>>         I’d like to propose a row of smaller fixes where code is
> noted
> >>>>>>>>>>> down a
> >>>>>>>>>>> bit questionable.
> >>>>>>>>>>>         SAP’s quality process requires that we fix these in our
> internal
> >>>>>>>>>>> delivery,
> >>>>>>>>>>> and I
> >>>>>>>>>>>         Would like to share my fixes with openJdk.  Some of these
> >> fixes
> >>>>>>>>>>> are of
> >>>>>>>>>>> more
> >>>>>>>>>>>         theoretical nature as how I understand the code paths
> never
> >>>>>>>>>>> allow the
> >>>>>>>>>>>         problematic situation, but fixing it nevertheless assures
> that
> >>>>>>>>>>> nothing is
> >>>>>>>>>>>         overseen if the code changes.  Most changes are in
> >> libawt_xawt,
> >>>>>>>>>>> some
> >>>>>>>>>>>         are in libsunec.
> >>>>>>>>>>>
> >>>>>>>>>>>         I’d appreciate a review:
> >>>>>>>>>>>         http://cr.openjdk.java.net/~goetz/wr16/8170525-
> >>>> awt/webrev.01/
> >>>>>>>>>>>         Changes in detail:
> >>>>>>>>>>>
> >>>>>>>>>>>         awt_InputMethod.c:
> >>>>>>>>>>>
> >>>>>>>>>>>         One might overrun the 100 byte fixed-size string
> >>>>>>>>>>> statusWindow->status
> >>>>>>>>>>> by copying text->string.multi_byte without checking the length.
> >>>>>>>>>>>
> >>>>>>>>>>>         gtk3_interface.c:
> >>>>>>>>>>>
> >>>>>>>>>>>         This less-than-zero comparison of an unsigned value is
> never
> >>>> true.
> >>>>>>>>>>>         Using uninitialized value color. Field color.alpha is
> >>>>>>>>>>> uninitialized.
> >>>>>>>>>>>         E.g. used at gtk3_interface.c:2287.
> >>>>>>>>>>>
> >>>>>>>>>>>         XToolkit.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value ret_timeout.
> >>>>>>>>>>>         E.g. in XToolkit.c:6809.
> >>>>>>>>>>>
> >>>>>>>>>>>         XWindow.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Argument is incompatible with corresponding format
> string
> >>>>>>>>>>> conversion.
> >>>>>>>>>>>
> >>>>>>>>>>>         splashscreen_sys.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Overflowed or truncated value (or a value computed from
> an
> >>>>>>>>>>> overflowed or truncated value) (gdk_scale>  0) ? native_scale *
> >>>>>>>>>>> (double)gdk_scale : native_scale used as return value.
> >>>>>>>>>>>
> >>>>>>>>>>>         ec.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value k.dp when calling mp_clear.
> >>>>>>>>>>>
> >>>>>>>>>>>         ecdecode.c
> >>>>>>>>>>>
> >>>>>>>>>>>         You might overrun the 291 byte fixed-size string genenc by
> >>>> copying
> >>>>>>>>>>> curveParams->geny without checking the length.
> >>>>>>>>>>>         Added sanity check before doing the string concatenation.
> >>>>>>>>>>>
> >>>>>>>>>>>         ecl_mult.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value kt.flag when calling
> >>>>>>>>>>> *group->point_mul. (The
> >>>>>>>>>>> function pointer resolves to ec_GF2m_pt_mul_mont.)
> >>>>>>>>>>>
> >>>>>>>>>>>         mpi.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value s. Field s.flag is uninitialized when
> >>>>>>>>>>> calling
> >>>>>>>>>>> s_mp_exch.
> >>>>>>>>>>>         Using uninitialized value tmp. Field tmp.flag is uninitialized
> >> when
> >>>>>>>>>>> calling s_mp_exch
> >>>>>>>>>>>         Using uninitialized value t.dp when calling mp_clear.
> >>>>>>>>>>>
> >>>>>>>>>>>         p11_mutex.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value *ckpInitArgs. Field ckpInitArgs-
> >flags
> >> is
> >>>>>>>>>>> uninitialized when calling memcpy.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>         DataBufferNative.c
> >>>>>>>>>>>
> >>>>>>>>>>>         Using uninitialized value lockInfo.rasBase when calling
> >>>>>>>>>>> BN_GetPixelPointer.
> >>>>>>>>>>>
> >>>>>>>>>>>         fontpath.c
> >>>>>>>>>>>
> >>>>>>>>>>>         You might overrun the 512 byte fixed-size string
> fontDirPath
> >> by
> >>>>>>>>>>> copying
> >>>>>>>>>>> DirP->name[index] without checking the length.
> >>>>>>>>>>>


More information about the 2d-dev mailing list