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.pakhnus...@oracle.com] > Sent: Friday, July 22, 2016 9:09 AM > To: Phil Race <philip.r...@oracle.com>; Lindenmaier, Goetz > <goetz.lindenma...@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.r...@oracle.com] > >>> Sent: Mittwoch, 20. Juli 2016 23:20 > >>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com> > >>> Cc: 'Vadim Pakhnushev' <vadim.pakhnus...@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.pakhnus...@oracle.com] > >>> Sent: Wednesday, July 20, 2016 7:14 PM > >>> To: Philip Race <philip.r...@oracle.com> > >>> <mailto:philip.r...@oracle.com> ; Lindenmaier, Goetz > >>> <goetz.lindenma...@sap.com> <mailto:goetz.lindenma...@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. > >>> > >>> > >>> > >>> > >>> > >
