gnuplot-beta

mouse.c (event_reset) bug

[<tim...@en...>]

Hi !

While working on my wxwidgets terminal, I've just encountered the=20
following problem : typing "set terminal wxt" twice (without any plot=20
call between) gives a segmentation fault.

Indeed, it calls event_reset to cancel zoombox (in particular) as=20
explained in a comment in set.c
event_reset calls term->set_cursor but this fails as the terminal is not=20
initialiased at this stage. I don't think it's a problem from my=20
terminal, so I am proposing the attached patch to check for=20
term_initialised.

I hope this can be included !

Greetings,
Timoth=E9e Lecomte

Re: mouse.c (event_reset) bug

[Ethan M. <merritt@u.washington.edu>]

On Friday 15 July 2005 03:17 pm, Timoth=C3=A9e Lecomte wrote:
> While working on my wxwidgets terminal, I've just encountered the=20
> following problem : typing "set terminal wxt" twice (without any plot=20
> call between) gives a segmentation fault.

That is indeed a problem, but since existing drivers do not segfault
on two "set term" command I think this is a something you need to
fix in your driver.
=20
> Indeed, it calls event_reset to cancel zoombox (in particular) as=20
> explained in a comment in set.c
> event_reset calls term->set_cursor but this fails as the terminal is not=
=20
> initialiased at this stage. I don't think it's a problem from my=20
> terminal, so I am proposing the attached patch to check for=20
> term_initialised.

=2D-- mouse.c	2005-07-16 00:01:58.000000000 +0200
+++ mouse2.c	2005-07-16 00:02:51.000000000 +0200
@@ -1782,7 +1782,7 @@
     modifier_mask =3D 0;
     button =3D 0;
     builtin_cancel_zoom(ge);
=2D    if (term && term->set_cursor) {
+    if (term && term->set_cursor && term_initialised) {
 	term->set_cursor(0, 0, 0);
 	if (mouse_setting.annotate_zoom_box && term->put_tmptext) {
 	    term->put_tmptext(1, "");


I am not certain this fix is corrent, since
term->set_cursor() is also called from builtin_cancel_zoom()
on the line above your new test.

Would it not be better to make sure your WXWIDGETS_set_cursor()
routine does not segfault.  Here is the routine for x11, where you
can see that if the communication channel is not initialized then
nothing happens:

TERM_PUBLIC void
X11_set_cursor(int c, int x, int y)
{
    if (X11_ipc) {
        PRINT3("u%04d%04d%04d\n", c, x, y);
        FFLUSH();
    }
}




=2D-=20
Ethan A Merritt       merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195

Re: mouse.c (event_reset) bug

[<tim...@en...>]

Ethan Merritt wrote:

>On Friday 15 July 2005 03:17 pm, Timoth=C3=A9e Lecomte wrote:
> =20
>
>>While working on my wxwidgets terminal, I've just encountered the=20
>>following problem : typing "set terminal wxt" twice (without any plot=20
>>call between) gives a segmentation fault.
>>   =20
>>
>
>That is indeed a problem, but since existing drivers do not segfault
>on two "set term" command I think this is a something you need to
>fix in your driver.
>=20
> =20
>
> (...)
>
>--- mouse.c	2005-07-16 00:01:58.000000000 +0200
>+++ mouse2.c	2005-07-16 00:02:51.000000000 +0200
>@@ -1782,7 +1782,7 @@
>     modifier_mask =3D 0;
>     button =3D 0;
>     builtin_cancel_zoom(ge);
>-    if (term && term->set_cursor) {
>+    if (term && term->set_cursor && term_initialised) {
> 	term->set_cursor(0, 0, 0);
> 	if (mouse_setting.annotate_zoom_box && term->put_tmptext) {
> 	    term->put_tmptext(1, "");
>
>
>I am not certain this fix is corrent, since
>term->set_cursor() is also called from builtin_cancel_zoom()
>on the line above your new test.
> =20
>
No, builtin_cancel_zoom returns before calling term->set_cursor()=20
because of the following test :

    *if* (!setting_zoom_region)
	*return* (char *) 0;



>Would it not be better to make sure your WXWIDGETS_set_cursor()
>routine does not segfault.  Here is the routine for x11, where you
>can see that if the communication channel is not initialized then
>nothing happens:
>
> =20
>
Of course, I can make such a test. And I will. I thought it would be=20
easier to fix the calling path ;-)

Greetings,

Timoth=C3=A9e

Re: mouse.c (event_reset) bug

[<tim...@en...>]

Ethan Merritt wrote:

>On Friday 15 July 2005 04:15 pm, Timoth=C3=A9e Lecomte wrote:
> =20
>
>>>I am not certain this fix is corrent, since
>>>term->set_cursor() is also called from builtin_cancel_zoom()
>>>
>>>     =20
>>>
>>No, builtin_cancel_zoom returns before calling term->set_cursor()=20
>>because of the following test :
>>
>>    *if* (!setting_zoom_region)
>>	*return* (char *) 0;
>>   =20
>>
>
>For x11 one could imagine that the plot window was closed in the middle
>of doing a zoom. In this case the test you quote above would not stop
>X11_set_cursor from being called, and since closing the plot window may
>have shut down the communication channel we still must test it
>explicitly inside X11_set_cursor.  I do not know if a similar sequence
>of events is possible with your new driver.
> =20
>
I have chosen to just hide the terminal window when the user closes it=20
via the menu or the window manager. Thus, it is still available to=20
answer to such calls.

> =20
>
>>Of course, I can make such a test. And I will. I thought it would be=20
>>easier to fix the calling path ;-)
>>   =20
>>
>
>I understood that.  I am just pointing out that if you truly want
>to protect the calling path, a different fix may be needed.
> =20
>
Ok, I understand. No problem !

Timoth=C3=A9e