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

[Lindenmaier, Goetz]

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.
> >>>
> >>
> >
> >