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.r...@oracle.com] > Sent: Monday, December 05, 2016 7:26 PM > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Sergey Bylokhov > <sergey.bylok...@oracle.com> > Cc: awt-...@openjdk.java.net; 2d-dev <2d-dev@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.r...@oracle.com] > >> Sent: Friday, December 02, 2016 8:46 PM > >> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Sergey Bylokhov > >> <sergey.bylok...@oracle.com> > >> Cc: awt-...@openjdk.java.net; 2d-dev <2d-dev@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.r...@oracle.com] > >>>> Sent: Donnerstag, 1. Dezember 2016 22:31 > >>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Sergey > Bylokhov > >>>> <sergey.bylok...@oracle.com>; Vincent Ryan > >> <vincent.x.r...@oracle.com> > >>>> Cc: awt-...@openjdk.java.net; 2d-dev <2d-dev@openjdk.java.net>; > >> security- > >>>> d...@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.r...@oracle.com] > >>>>>> Sent: Mittwoch, 30. November 2016 20:59 > >>>>>> To: Sergey Bylokhov <sergey.bylok...@oracle.com>; Lindenmaier, > >> Goetz > >>>>>> <goetz.lindenma...@sap.com>; Vincent Ryan > >> <vincent.x.r...@oracle.com> > >>>>>> Cc: awt-...@openjdk.java.net; 2d-dev <2d-dev@openjdk.java.net>; > >> security- > >>>>>> d...@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.r...@oracle.com] > >>>>>>>>> Sent: Mittwoch, 30. November 2016 14:53 > >>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com> > >>>>>>>>> Cc: awt-...@openjdk.java.net; security-...@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.lindenma...@sap.com > >> <mailto:goetz.lindenma...@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. > >>>>>>>>>
