<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux

Anton Litvinov anton.litvinov at oracle.com
Fri Feb 15 04:29:27 PST 2013

Hello Anthony,

Thank you very much for approval of this fix.

Thank you,

On 2/15/2013 4:18 PM, Anthony Petrov wrote:
> Hi Anton,
> The fix looks great to me. Thanks for all your hard work!
> -- 
> best regards,
> Anthony
> On 2/13/2013 20:57, Anton Litvinov wrote:
>>   Hello Anthony,
>> Could you please review the third version of the fix containing 
>> modifications discussed with you in the previous letter.
>> Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.02
>> This version of the fix differs from the previous in the following 
>> places:
>>    1. A comment about the place of invocation of the method
>>       "XErrorHandlerUtil.init" was added to a documentation block of the
>>       method.
>>    2. A code related to XShmAttach function common to the files
>>       "src/solaris/native/sun/awt/awt_GraphicsEnv.c" and
>>       "src/solaris/native/sun/java2d/x11/X11SurfaceData.c" was extracted
>>       into a separate function "TryXShmAttach" declared in
>>       "src/solaris/native/sun/awt/awt_GraphicsEnv.h" file.
>>    3. All JNI code related to X error handling was implemented as
>>       corresponding macros defined in
>>       "src/solaris/native/sun/awt/awt_util.h" file.
>> Thank you,
>> Anton
>> On 1/31/2013 7:42 PM, Anton Litvinov wrote:
>>> Hello Anthony,
>>> Thank you for the review and these remarks. Surely, the comment will 
>>> be added. I think that all JNI code related to XShmAttach can be 
>>> definitely transferred into a separate dedicated function, which 
>>> will be declared in "src/solaris/native/sun/awt/awt_GraphicsEnv.h" 
>>> file. I will try to wrap all JNU calls connected with XErrorHandler 
>>> functions or macros.
>>> Thank you,
>>> Anton
>>> On 1/31/2013 4:57 PM, Anthony Petrov wrote:
>>>> Hi Anton,
>>>> A couple comments:
>>>> 1. src/solaris/classes/sun/awt/X11/XErrorHandlerUtil.java
>>>>>   80     private static void init(long display) {
>>>> This method is private and isn't called from anywhere in this class 
>>>> itself. This looks confusing. Please add a comment stating that 
>>>> this method is invoked from native code, and from where exactly.
>>>> 2. Interesting that we use this machinery to call the XShmAttach() 
>>>> from native code twice, and the code looks quite similar in each 
>>>> case. Would it be possible to extract the common code in a separate 
>>>> function (a-la BOOL TryXShmAttach(...)) to avoid code replication? 
>>>> There are other usages as well, so we could also introduce a macro 
>>>> (such as the old EXEC_WITH_XERROR_HANDLER but now with other 
>>>> arguments) that would minimize all the JNU_ calls required to use 
>>>> this machinery.
>>>> Otherwise the fix looks great.
>>>> -- 
>>>> best regards,
>>>> Anthony
>>>> On 1/30/2013 20:14, Anton Litvinov wrote:
>>>>>   Hello Anthony,
>>>>> Could you, please, review a second version of the fix, which is 
>>>>> based on an idea of reusing the existing AWT native global error 
>>>>> handler from Java 2D native code.
>>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.01
>>>>> The fix consists of the following parts:
>>>>>    1. Migration of all X error handling code from XToolkit to a new
>>>>>       XErrorHandlerUtil class for resolution of interdependency 
>>>>> between
>>>>>       a static initialization block of XToolkit and a block 
>>>>> initializing
>>>>>       java.awt.GraphicsEnvironment singleton. Such dependency is 
>>>>> created
>>>>>       by new calls to XToolkit static methods from
>>>>>       "src/solaris/native/sun/awt/awt_GraphicsEnv.c",
>>>>>       "src/solaris/native/sun/java2d/x11/X11SurfaceData.c" files.
>>>>>    2. Substitution of XToolkit.WITH_XERROR_HANDLER,
>>>>>       XToolkit.RESTORE_XERROR_HANDLER ... for corresponding methods,
>>>>>       fields of XErrorHandlerUtil class in all places of JDK source
>>>>>       code, where they were used.
>>>>>    3. Substitution of all found native X error handlers which are 
>>>>> set in
>>>>>       native code (awt_GraphicsEnv.c, X11SurfaceData.c,
>>>>>       GLXSurfaceData.c) for new synthetic Java error handlers.
>>>>>    4. Removal of X error handling code used by the native error 
>>>>> handlers
>>>>>       from "solaris/native/sun/awt/awt_util.c"
>>>>>       "solaris/native/sun/awt/awt_util.h" files.
>>>>> Thank you,
>>>>> Anton
>>>>> On 1/11/2013 3:45 PM, Anthony Petrov wrote:
>>>>>> I'm not Jim, but as I indicated earlier my opinion is that the 
>>>>>> easiest way to fix this is to install the existing 
>>>>>> J2DXErrHandler() only once. That is, it is the second option 
>>>>>> listed by you. Of course, the J2DXErrHandler needs to be updated 
>>>>>> as well to detect whether 2D code wants to use it at the moment 
>>>>>> or it must simply delegate to the previous handler (i.e. where 
>>>>>> the code currently installs/uninstalls the handler, it must 
>>>>>> instead set a global boolean flag or something.)
>>>>>> While the first option (reusing the existing AWT machinery) is an 
>>>>>> interesting idea in general, I think it is complex and would 
>>>>>> require too much additional testing and bring an unjustified risk 
>>>>>> to the solution for such a basic problem.
>>>>>> -- 
>>>>>> best regards,
>>>>>> Anthony
>>>>>> On 1/11/2013 14:44, Anton Litvinov wrote:
>>>>>>> Hello Jim,
>>>>>>> Thank you very much for the review and provision of a new idea 
>>>>>>> of a solution. Elimination of the logic, which sets/unsets 
>>>>>>> J2DXErrHandler() for each call "XShmAttach(awt_display, 
>>>>>>> &shminfo))" should effectively resolve the issue, but only in 
>>>>>>> case if all other native error handlers, which were set by the 
>>>>>>> system function "XSetErrorHandler()" in JDK or in any external 
>>>>>>> library, observe the rule of relaying of all events, which are 
>>>>>>> not relative to them, to the previously saved error handlers. 
>>>>>>> Otherwise an error generated during "XShmAttach" function call 
>>>>>>> will not be handled by J2DXErrHandler().
>>>>>>> Could you answer the following question. By setting 
>>>>>>> J2DXErrHandler() only once and forever do you mean usage of AWT 
>>>>>>> global event handler "static int ToolkitErrorHandler(Display * 
>>>>>>> dpy, XErrorEvent * event)" from 
>>>>>>> "src/solaris/native/sun/xawt/XlibWrapper.c" with Java synthetic 
>>>>>>> handlers or creation of another global native error handler with 
>>>>>>> J2DXErrHandler as native synthetic handler?
>>>>>>> Thank you,
>>>>>>> Anton
>>>>>>> On 1/10/2013 5:44 AM, Jim Graham wrote:
>>>>>>>> I think I'd rather see some way to prevent double-adding the 
>>>>>>>> handler in the first place as well.  Since it is only ever used 
>>>>>>>> on errors I also think it is OK to set it once and leave it 
>>>>>>>> there forever...
>>>>>>>>             ...jim
>>>>>>>> On 1/9/13 8:08 AM, Anthony Petrov wrote:
>>>>>>>>> Hi Anton et al.,
>>>>>>>>> If I read the description of the bug correctly, specifically 
>>>>>>>>> this part:
>>>>>>>>>> The problem occurs, if another thread (for example, GTK 
>>>>>>>>>> thread) is
>>>>>>>>>> doing the same sort of thing concurrently. This can lead to the
>>>>>>>>>> following situation.
>>>>>>>>>>  JVM thread: Sets J2DXErrHandler(), saves 
>>>>>>>>>> previous      GTK thread: Sets some GTK_HANDLER, saves
>>>>>>>>>> J2DXErrHandler() as previous  JVM thread: Restores
>>>>>>>>>> ANY_PREVIOUS_HANDLER      GTK thread: Restores 
>>>>>>>>>> J2DXErrHandler()  JVM
>>>>>>>>>> thread: Sets J2DXErrHandler(), saves J2DXErrHandler() as 
>>>>>>>>>> previous
>>>>>>>>> It is obvious that at this final step 2D is in an inconsistent 
>>>>>>>>> state. We
>>>>>>>>> don't expect to replace our own error handler (and it 
>>>>>>>>> shouldn't have
>>>>>>>>> been there in the first place).
>>>>>>>>> I realize that the fix you propose works around this problem. 
>>>>>>>>> But this
>>>>>>>>> doesn't look like an ideal solution to me.
>>>>>>>>> BTW, IIRC, in JDK7 (and 6?) we decided to set the actual X11 
>>>>>>>>> error
>>>>>>>>> handler only once and never replace it. All the rest of the
>>>>>>>>> push_handler/pop_handler logic is now located in Java code (see
>>>>>>>>> XToolkit.SAVED_ERROR_HANDLER() and the surrounding logic). I 
>>>>>>>>> believe
>>>>>>>>> that we should somehow share this machinery with the 2D code 
>>>>>>>>> to avoid
>>>>>>>>> this sort of problems. Though I'm not sure if this will 
>>>>>>>>> eliminate this
>>>>>>>>> exact issue.
>>>>>>>>> 2D/AWT folks: any other thoughts?
>>>>>>>>> -- 
>>>>>>>>> best regards,
>>>>>>>>> Anthony
>>>>>>>>> On 12/29/2012 17:44, Anton Litvinov wrote:
>>>>>>>>>> Hello,
>>>>>>>>>> Please review the following fix for a bug.
>>>>>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005607
>>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8005607
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.00
>>>>>>>>>> The bug consists in a crash which is caused by a stack 
>>>>>>>>>> overflow for
>>>>>>>>>> the reason of an infinite recursion in AWT native function
>>>>>>>>>> J2DXErrHandler() under certain circumstances on 32-bit Linux 
>>>>>>>>>> OS. The
>>>>>>>>>> fix is based on introduction of the logic, which detects 
>>>>>>>>>> indirect
>>>>>>>>>> recursive calls to J2DXErrHandler() by means of a simple 
>>>>>>>>>> counter, to
>>>>>>>>>> J2DXErrHandler() native function. Such a solution requires 
>>>>>>>>>> minimum
>>>>>>>>>> code changes, does not alter the handler's code significantly 
>>>>>>>>>> and
>>>>>>>>>> eliminates this bug.
>>>>>>>>>> Adding 2d-dev at openjdk.java.net e-mail alias to the list of 
>>>>>>>>>> recipients
>>>>>>>>>> of this letter, because the edited function's name is related 
>>>>>>>>>> to Java
>>>>>>>>>> 2D area of JDK, despite of the fact that the edited file is 
>>>>>>>>>> located in
>>>>>>>>>> AWT directory.
>>>>>>>>>> Thank you,
>>>>>>>>>> Anton

More information about the awt-dev mailing list