<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux
Artem Ananiev
artem.ananiev at oracle.com
Wed Feb 20 05:43:49 PST 2013
Looks fine.
Thanks,
Artem
On 2/18/2013 8:08 PM, Anton Litvinov wrote:
> Hello Artem,
>
> Could you please review a new version of the fix. The method
> "XErrorHandlerUtil.getDisplay()" was removed and
> "XErrorHandlerUtil.XSync()" method refers to the field "display"
> directly now.
>
> Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.03
>
> Thank you,
> Anton
>
> On 2/18/2013 4:23 PM, Artem Ananiev wrote:
>>
>> On 2/18/2013 4:04 PM, Anton Litvinov wrote:
>>> Hello Artem,
>>>
>>> Thank you very much for the review of this fix. My responses to your
>>> questions are provided below in the same order, which you defined.
>>>
>>> 1. I think that "XErrorHandlerUtil.saved_error" field can surely be
>>> marked as private, but in this case the corresponding
>>> "XErrorHandlerUtil.getSavedError" method will be necessary, because
>>> this field is actively accessed from other classes which set a
>>> certain instance of XErrorHandler. For example
>>> "MotifDnDDropTargetProtocol.java", "XDragSourceProtocol.java" and a
>>> few other classes edited in this fix.
>>
>> OK, I missed that usages when looking at the webrev. Let it stay
>> unchanged now.
>>
>>> 2. Yes, I completely agree that "XErrorHandlerUtil.getDisplay()" is
>>> reduntant. This method will be eliminated.
>>
>> Thanks,
>>
>> Artem
>>
>>> Thank you,
>>> Anton
>>>
>>> On 2/18/2013 3:16 PM, Artem Ananiev wrote:
>>>> Hi, Anton,
>>>>
>>>> a few minor comments:
>>>>
>>>> 1. XErrorHandlerUtil: can saved_error be private instead of package
>>>> protected?
>>>>
>>>> 2. XErrorHandlerUtil.getDisplay() seems to be redundant.
>>>>
>>>> In general, the fix looks perfectly fine to me. Please, wait for
>>>> comments from Java2D team, though.
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 2/13/2013 8:57 PM, 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 into the
>>>>>> particular "WITH_XERROR_HANDLER", "RESTORE_XERROR_HANDLER" 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
>>>>>>>>>>>>> ANY_PREVIOUS_HANDLER as
>>>>>>>>>>>>> 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