[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> RFR(S): 8161923: Fix two memory issues.
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2016-07-22 19:16:20
Message-ID: 4cd5960851a04ce18d480573216ac0e6 () DEWDFE13DE09 ! global ! corp ! sap
[Download RAW message or body]
Hi,
now we have a row of solutions ... I would like
to finish this up, please.
Could I just keep the version right now? Or which one?
Also we lost the mailinglists for some reason.
For the records: I had posted webrev02:
http://cr.openjdk.java.net/~goetz/wr16/8161923-jdkMem/webrev.02/
Best regards,
Goetz.
> -----Original Message-----
> From: Vadim Pakhnushev [mailto:vadim.pakhnushev@oracle.com]
> Sent: Friday, July 22, 2016 9:09 AM
> To: Phil Race <philip.race@oracle.com>; Lindenmaier, Goetz
> <goetz.lindenmaier@sap.com>
> Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> RFR(S): 8161923: Fix two memory
> issues.
>
> I'd do similar to what is already in the file:
>
> if (++m >= nComponents) {
> m = 0;
> }
> componentStack[m] = currGlyph;
>
> So in this case:
>
> if (++mm == nComponents) {
> LE_DEBUG_BAD_FONT("exceeded nComponents");
> mm--; // don't overrun the stack.
> // or mm = nComponents-1;
> }
> stack[mm] = componentGlyph;
>
> Vadim
>
> On 22.07.2016 1:00, Phil Race wrote:
> > On 07/21/2016 02:40 AM, Lindenmaier, Goetz wrote:
> >> Hi Phil,
> >>
> >> actually I must say that I (and a colleague of mine) had to look at what
> >> you propose quite a while to see it's correct.
> >
> > Hmm .. surprising. Vadim, which do you think is clearer ?
> >
> > -phil.
> >
> >
> >> Not so with the mm++
> >> before the check. The extra increment/decrement should be optimized
> >> by the C++ compiler I guess.
> >> But both require a comment that says that the last stack element
> >> is overvritten in case of overflow, I think.
> >>
> >> Yes, these fixes originally stem from a Coverity run.
> >> But that was a run on jdk8 when we released that. Currently, another
> >> colleague of mine is moving our fixes from jdk8 to jdk9. There
> >> he spotted these two issues, and we thought they would be useful
> >> for the open community.
> >>
> >> Best regards,
> >> Goetz.
> >>
> >>
> >>> -----Original Message-----
> >>> From: Phil Race [mailto:philip.race@oracle.com]
> >>> Sent: Mittwoch, 20. Juli 2016 23:20
> >>> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>
> >>> Cc: 'Vadim Pakhnushev' <vadim.pakhnushev@oracle.com>
> >>> Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> RFR(S): 8161923: Fix two
> memory
> >>> issues.
> >>>
> >>> If we are going to refactor this why not make it more
> >>> straightforward :-
> >>>
> >>>
> >>> if (m+1 >= nComponents) {
> >>> LE_DEBUG_BAD_FONT("exceeded nComponents");
> >>> } else {
> >>> m++;
> >>> }
> >>> stack[mm] = componentGlyph;
> >>>
> >>>
> >>>
> >>> Out of curiousity ... I was aware of the other file too .. and was
> >>> surprised a bit
> >>> that there
> >>> was an update to one file and not the other since I supposed this was
> >>> discovered
> >>> by a tool which would catch the pattern in both cases.
> >>>
> >>> But only catching one seems like "by eye" .. except that seems hard to
> >>> believe.
> >>>
> >>> So I am concluding it was a tool that found this as an actual
> >>> runtime overflow
> >>> and only the one code path was executed .. which is not that
> >>> surprising since
> >>> only the "2" path is taken by all modern fonts. But only Apple make
> >>> such
> >>> fonts
> >>> which suggest you were running on OS X
> >>>
> >>> So how and where was it found ?
> >>>
> >>>
> >>> The windows one is interesting .. I am a bit surprised that hasn't
> >>> caused a
> >>> crash and
> >>> I suppose windows is smart enough to know its wrong and ignore it.
> >>>
> >>>
> >>> -phil.
> >>>
> >>>
> >>>
> >>> On 07/20/2016 01:45 PM, Lindenmaier, Goetz wrote:
> >>>
> >>>
> >>> Hi Vadim and Phil,
> >>>
> >>>
> >>>
> >>> thanks for looking at this.
> >>>
> >>>
> >>>
> >>> Yes Vadim, what you say is how I uunderstand it.
> >>>
> >>>
> >>>
> >>> Basically the code is like this, let's assume mm==nComponents
> >>>
> >>> if (mm==nComponents) { // true
> >>>
> >>> LE_DEBUG_BAD_FONT("exceeded nComponents");
> >>>
> >>> mm--; // don't overrun the stack. // mm =
> >>> (nComponents-1)
> >>>
> >>> }
> >>>
> >>> ++mm; // mm =
> >>> (nComponents-1)+1 =
> >>> nComponents
> >>>
> >>> stack[mm] = componentGlyph; // stack[nComponents]
> >>>
> >>>
> >>>
> >>> Changing the if as you propose, Vadim, should work. But actually, I
> >>> would
> >>>
> >>> move the ++mm into a line of it's own, obviously it's hard to
> >>> understand
> >>>
> >>> if it's in the condition.
> >>>
> >>>
> >>>
> >>> New webrev, also fixing the other file:
> >>>
> >>> http://cr.openjdk.java.net/~goetz/wr16/8161923-
> >>> jdkMem/webrev.02/
> <http://cr.openjdk.java.net/%7Egoetz/wr16/8161923-
> >>> jdkMem/webrev.02/>
> >>>
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Goetz.
> >>>
> >>>
> >>>
> >>> From: Vadim Pakhnushev [mailto:vadim.pakhnushev@oracle.com]
> >>> Sent: Wednesday, July 20, 2016 7:14 PM
> >>> To: Philip Race <philip.race@oracle.com>
> >>> <mailto:philip.race@oracle.com> ; Lindenmaier, Goetz
> >>> <goetz.lindenmaier@sap.com> <mailto:goetz.lindenmaier@sap.com>
> >>> Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> RFR(S): 8161923: Fix two
> >>> memory issues.
> >>>
> >>>
> >>>
> >>> Phil,
> >>>
> >>> I was wondering the same, but after looking more closely I realized
> >>> that if mm equals nComponents it would be decremented and then
> >>> preincremented and the stack would be written at index nComponents.
> >>>
> >>> My suggestion is to move increment here:
> >>> if(++mm==nComponents) {
> >>> ...
> >>> stack[mm] = componentGlyph;
> >>>
> >>> Thanks,
> >>> Vadim
> >>>
> >>> On 20.07.2016 19:58, Philip Race wrote:
> >>>
> >>> Goetz,
> >>>
> >>> Can you illustrate exactly how this overwrite can occur ?
> >>> I see checks that ensure that if mm==nComponents it is reset
> >>> ..
> >>>
> >>> -phil.
> >>>
> >>> On 7/20/16, 8:36 AM, Vadim Pakhnushev wrote:
> >>>
> >>> Hi Goetz,
> >>>
> >>> Maybe instead of increasing the stack size we could
> >>> move the increment from the assignment to the previous if statement
> >>> where we check for the overwrite possibility?
> >>> There are similar code patterns in this file.
> >>> Also there is almost identical file
> >>> LigatureSubstProc.cpp which also contains similar code.
> >>>
> >>> Thanks,
> >>> Vadim
> >>>
> >>> On 20.07.2016 16:13, Lindenmaier, Goetz wrote:
> >>>
> >>> Hi
> >>>
> >>>
> >>>
> >>> This changes fixes two memory issues.
> >>>
> >>>
> >>>
> >>> In awt_PrintControl.cpp, a wrong pointer is
> >>> freed.
> >>>
> >>> In LigatureSubstProc2.cpp, line 157:
> >>>
> >>> stack[++mm] = componentGlyph;
> >>>
> >>> can overwrite the stack by one element. It will
> >>> write
> >>>
> >>> stack[nComponents], because ++mm
> >>> increments before
> >>>
> >>> accessing the array.
> >>>
> >>>
> >>>
> >>> Fix: increase the size of the array by one.
> >>>
> >>>
> >>>
> >>> Please review this change:
> >>>
> >>>
> >>> http://cr.openjdk.java.net/~goetz/wr16/8161923-
> >>> jdkMem/webrev.01/
> <http://cr.openjdk.java.net/%7Egoetz/wr16/8161923-
> >>> jdkMem/webrev.01/>
> >>>
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Goetz.
> >>>
> >>>
> >>>
> >>>
> >>>
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic