Hi all,
In our recent troubleshooting trying to hunt down a mysterious crash at patch closing we uncovered a bug that appears to be present in pd-extended 0.41.4 and 0.42.5, as well as pd vanilla (svn checked out yesterday). Following is reproducible on Linux 9.04 Ubuntu i386.
To reproduce the crash if you run the patch included below (pretty much vanilla version of netsend/netreceive) as follows:
#N canvas 511 186 450 322 10; #X floatatom 80 -44 0 0 0 0 - - -; #X floatatom 71 32 0 0 0 0 - - -; #X obj 125 -117 tgl 15 0 empty empty empty 17 7 0 10 -262144 -1 -1 0 1; #X obj 74 -177 loadbang; #X obj 71 5 netsend 1; #X floatatom 212 -18 5 0 0 0 - - -; #X obj 214 -44 netreceive 3000 1; #X msg 80 -17 send $1; #X msg 71 -157 connect 192.168.1.8 3000; #X obj 122 -95 metro 2; #X obj 151 -53 +; #X msg 114 -69 1; #X connect 0 0 7 0; #X connect 0 0 10 1; #X connect 2 0 9 0; #X connect 3 0 8 0; #X connect 4 0 1 0; #X connect 6 0 5 0; #X connect 7 0 4 0; #X connect 8 0 4 0; #X connect 9 0 11 0; #X connect 10 0 0 0; #X connect 11 0 10 0;
1) save the patch and click on "connect" message 2) start the metro 3) close the patch *without* stopping the metro 4) reopen it again, and you'll find pd reporting that the old connection still exists (explained below) 5) at this point you can start the metro again (it should work) 6) however, when closing the patch pd will crash
Part of the problem:
x->x_fd in netsend_disconnect is the same flag used in netsend_send function, so when one tries to close socket but there is still a stream of data coming in, because x->x_fd is reverted to -1 after the attempt to close the socket, the data keeps coming in through netsend_send function potentially (and I speculate here) preventing socket from closing. This leaves a stale open socket and eventually causes the whole thing to crash.
Below is the problematic code.
static void netsend_disconnect(t_netsend *x) { if (x->x_fd >= 0) { sys_closesocket(x->x_fd); x->x_fd = -1; outlet_float(x->x_obj.ob_outlet, 0); } }
Solution is changing the netsend_disconnect to create a temp value from the x->x_fd, change x->x_fd to -1 to prevent other incoming data from being processed, and only then try to close the socket.
This has apparently fixed part of the problem on our end here.
Please see x_net.c patch below:
--- x_net.c.old 2009-11-18 17:45:24.000000000 -0500 +++ x_net.c 2009-11-18 17:41:38.000000000 -0500 @@ -106,8 +106,9 @@ { if (x->x_fd >= 0) { - sys_closesocket(x->x_fd); - x->x_fd = -1; + int tmp = x->x_fd; + x->x_fd = -1; + sys_closesocket(tmp); outlet_float(x->x_obj.ob_outlet, 0); } }
The problem is this fixes crashing in a non-rt version of Pd. The rt version however still suffers sporadically from the same problem. It may take several tries to instigate it, but when it finally occurs, in my case the reopening of the patch at first misbehaves in that whatever I do on canvas with a mouse or keyboard, everything is being reported twice (e.g. clicking on the toggle creates an on and an off event, creating a new object creates two, typing into it creates 2 letters per each keypress, etc.). After that closing the patch crashes pd. I've had this occur before as well but now at least the other bug with open socket is not there any more.
Any ideas as to why?
Ico
The problem is this fixes crashing in a non-rt version of Pd. The rt version however still suffers sporadically from the same problem. It may take several tries to instigate it, but when it finally occurs, in my case the reopening of the patch at first misbehaves in that whatever I do on canvas with a mouse or keyboard, everything is being reported twice (e.g. clicking on the toggle creates an on and an off event, creating a new object creates two, typing into it creates 2 letters per each keypress, etc.). After that closing the patch crashes pd. I've had this occur before as well but now at least the other bug with open socket is not there any more.
Any ideas as to why?
As a follow-up to this problem, manually stopping metro before closing the patch solves this problem, but that is not obviously the way to ensure someone will remember to do so every time before closing...
Ico
As a follow-up to this problem, manually stopping metro before closing the patch solves this problem, but that is not obviously the way to ensure someone will remember to do so every time before closing...
Another correction. This problem is not apparent in pd-vanilla (even though the source is the same) but it only affects the pd-extended. Bizzare...
ico
Yay! Finally figured it out.
Please disregard my netsend patch, it's basically treating symptoms rather than the source.
It turns out that the patch I submitted before to fix canvas GOP toggle on/apply/off/apply crash has been the cause of the problem all along mainly because the final version had the second part nested, causing a lot of problems at close time.
Namely, the old version is like this (prior to my patch):
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); editor_free(x->gl_editor, x); x->gl_editor = 0; } }
The currently broken patched version (the one I proposed earlier) is:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) glist_noselect(x); if (x->gl_editor && x->gl_list) { if (x->gl_list) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob));
//notice how this part is never reached if x->gl_list //condition is not satisfied, yet it is reached in //previous version... //this is the cause of //problems I've been experiencing if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } } } }
Sooooo, here's the correct version which still resolves two issues pointed out before without introducing any known regressions. The issues once again are:
#1 crash when creating a new patch->create a sub-patch->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply #2 crash when creating a new patch->create a sub-patch->create an object *and leave elected->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply
CORRECT VERSION:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; glist_noselect(x); //necessary to resolve crash #2 if (x->gl_editor) { if (x->gl_list) { //necessary to resolve crash #1 for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); } //fix to make this condition independent from x->gl_list if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } } }
Cheers!
ico
Hey Ico,
You should post this to the patch tracker once you are ready to submit it.
.hc
On Nov 18, 2009, at 11:40 PM, Ivica Ico Bukvic wrote:
Yay! Finally figured it out.
Please disregard my netsend patch, it's basically treating symptoms rather than the source.
It turns out that the patch I submitted before to fix canvas GOP toggle on/apply/off/apply crash has been the cause of the problem all along mainly because the final version had the second part nested, causing a lot of problems at close time.
Namely, the old version is like this (prior to my patch):
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); editor_free(x->gl_editor, x); x->gl_editor = 0; } }
The currently broken patched version (the one I proposed earlier) is:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) glist_noselect(x); if (x->gl_editor && x->gl_list) { if (x->gl_list) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob));
//notice how this part is never reached if x->gl_list //condition is not satisfied, yet it is reached in //previous version... //this is the cause of //problems I've been experiencing if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } }} }
Sooooo, here's the correct version which still resolves two issues pointed out before without introducing any known regressions. The issues once again are:
#1 crash when creating a new patch->create a sub-patch->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply #2 crash when creating a new patch->create a sub-patch->create an object *and leave elected->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply
CORRECT VERSION:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; glist_noselect(x); //necessary to resolve crash #2 if (x->gl_editor) { if (x->gl_list) { //necessary to resolve crash #1 for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); } //fix to make this condition independent from x->gl_list if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } } }
Cheers!
ico
----------------------------------------------------------------------------
Information wants to be free. -Stewart Brand
So adding glist_noselect(x); to canvas_destroy_editor() seems to make sense. The rest I think it better solved elsewhere. Basically canvas_destroy_editor() calls glist_findrtext() which calls canvas_create_editor():
http://lists.puredata.info/pipermail/pd-dev/2010-01/014734.html
.hc
On Nov 18, 2009, at 11:40 PM, Ivica Ico Bukvic wrote:
Yay! Finally figured it out.
Please disregard my netsend patch, it's basically treating symptoms rather than the source.
It turns out that the patch I submitted before to fix canvas GOP toggle on/apply/off/apply crash has been the cause of the problem all along mainly because the final version had the second part nested, causing a lot of problems at close time.
Namely, the old version is like this (prior to my patch):
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); editor_free(x->gl_editor, x); x->gl_editor = 0; } }
The currently broken patched version (the one I proposed earlier) is:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; if (x->gl_editor) glist_noselect(x); if (x->gl_editor && x->gl_list) { if (x->gl_list) { for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob));
//notice how this part is never reached if x->gl_list //condition is not satisfied, yet it is reached in //previous version... //this is the cause of //problems I've been experiencing if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } }} }
Sooooo, here's the correct version which still resolves two issues pointed out before without introducing any known regressions. The issues once again are:
#1 crash when creating a new patch->create a sub-patch->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply #2 crash when creating a new patch->create a sub-patch->create an object *and leave elected->open sub-patch properties->enable GOP->apply->disable GOP (without closing properties)->apply
CORRECT VERSION:
void canvas_destroy_editor(t_glist *x) { t_gobj *y; t_object *ob; glist_noselect(x); //necessary to resolve crash #2 if (x->gl_editor) { if (x->gl_list) { //necessary to resolve crash #1 for (y = x->gl_list; y; y = y->g_next) if (ob = pd_checkobject(&y->g_pd)) rtext_free(glist_findrtext(x, ob)); } //fix to make this condition independent from x->gl_list if (x->gl_editor) { editor_free(x->gl_editor, x); x->gl_editor = 0; } } }
Cheers!
ico
----------------------------------------------------------------------------
"[T]he greatest purveyor of violence in the world today [is] my own government." - Martin Luther King, Jr.
-
Hans-Christoph Steiner -
Ivica Ico Bukvic