<AWT Dev> [OpenJDK 2D-Dev] RFR(M): 8170525: Fix minor issues in awt coding
Phil Race
philip.race at oracle.com
Fri Dec 2 19:45:44 UTC 2016
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 awt-dev
mailing list