[OpenJDK 2D-Dev] <AWT Dev> RFR(M): 8170525: Fix minor issues in awt coding
Philip Race
philip.race at oracle.com
Tue Dec 6 03:20:42 UTC 2016
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