From: Evan S. <ev...@ad...> - 2006-09-13 21:55:26
|
I know Sean is doing server.c death meanuvers; this might be worth fixing in the process: Right now, most of the server.c functions look like: void serv_rem_deny(GaimConnection *g, const char *name) { GaimPluginProtocolInfo *prpl_info = NULL; if (g != NULL && g->prpl != NULL) prpl_info = GAIM_PLUGIN_PROTOCOL_INFO(g->prpl); if (prpl_info && g_list_find(gaim_connections_get_all(), g) && prpl_info->rem_deny) prpl_info->rem_deny(g, name); } bolding mine. If the GaimConnection isn't valid, there's a good chance that g->prpl is going to crash before that check even happens. The check is just slightly upping the chances that we don't crash -- we got away with it once, accessing freed memory, but this way we don't send it to the prpl to roll the dice even more. Either the GaimConnection is always valid, in which case that check is needless, or it isn't always valid, in which case it should be a g_return_if_fail(GAIM_CONNECTION_IS_VALID(gc)); or the like. I got this crash earlier while rapidly signing gadu-gadu offline and online: #0 serv_set_permit_deny (g=0xd4513a0) at /Users/evands/libgaim/ Libgaim/src/server.c:315 prpl_info = (struct _GaimPluginProtocolInfo *) 0x7999d74 #1 0x0773c938 in gaim_connection_set_state (gc=0xd4513a0, state=126271292) at /Users/evands/libgaim/Libgaim/src/connection.c:301 account = (struct _GaimAccount *) 0xd482700 presence = (struct _GaimPresence *) 0xd482800 ops = (GaimConnectionUiOps *) 0x375e034 #2 0x077521fc in ggp_async_login_handler (_gc=0xd4513a0, fd=0x786bf3c, cond=4278124287) at /Users/evands/libgaim/Libgaim/src/ protocols/gg/gg.c:1479 account = (struct _GaimAccount *) 0xd482700 status = (struct _GaimStatus *) 0x786bf3c gc = (struct _GaimConnection *) 0xd4513a0 info = (GGPInfo *) 0xd482700 ev = (struct gg_event *) 0xf1a8470 #3 <socket callback> which is a way of saying (1) that gadu-gadu may be freeing memory somewhere before unregistering its fd listener and (2) there's at least one existing codepath that can crash at present but wouldn't, I think, with the check higher up in the function. -Evan |
From: Mark D. <ma...@ki...> - 2006-09-17 17:12:43
|
On Wed, 13 Sep 2006 17:55:17 -0400, Evan Schoenberg wrote > I know Sean is doing server.c death meanuvers; this might be worth > fixing in the process: > > Right now, most of the server.c functions look like: > void serv_rem_deny(GaimConnection *g, const char *name) > { > GaimPluginProtocolInfo *prpl_info = NULL; > > if (g != NULL && g->prpl != NULL) > prpl_info = GAIM_PLUGIN_PROTOCOL_INFO(g->prpl); > > if (prpl_info && g_list_find(gaim_connections_get_all(), g) && > prpl_info->rem_deny) > prpl_info->rem_deny(g, name); > } > > bolding mine. > > If the GaimConnection isn't valid, there's a good chance that g- > >prpl is going to crash before that check even happens. The check > is just slightly upping the chances that we don't crash -- we got > away with it once, accessing freed memory, but this way we don't > send it to the prpl to roll the dice even more. > > Either the GaimConnection is always valid, in which case that check > is needless, or it isn't always valid, in which case it should be a > g_return_if_fail(GAIM_CONNECTION_IS_VALID(gc)); > or the like. FYI I hope to make the use of GAIM_CONNECTION_IS_VALID not necessary in as many places as possible. It's kind of a gross kludge, and shouldn't really be needed if timeouts are correctly canceled, watchers are destroyed, etc. So ideally someone will figure out why that check is needed. I have a feeling that will end up being me... but it doesn't have to be! I'm sure there are plenty of smashing programmers just dying to procrastinate on doing some schoolwork. What better way to procrastinate than to help your fellow Gaim user by fixing a bug? -Mark |
From: Adil <ad...@ya...> - 2006-09-17 18:58:44
|
--- Mark Doliner <ma...@ki...> wrote: > > FYI I hope to make the use of > GAIM_CONNECTION_IS_VALID not necessary in as > many places as possible. It's kind of a gross > kludge, and shouldn't really be > needed if timeouts are correctly canceled, watchers > are destroyed, etc. > Perhaps it's time to get started on GObjectification! Sadrul __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com |
From: Eduardo <ep...@us...> - 2006-09-18 10:42:28
|
On 2006-09-17 18:58:36 UTC, Adil wrote: > --- Mark Doliner <ma...@ki...> wrote: > > FYI I hope to make the use of > > GAIM_CONNECTION_IS_VALID not necessary in as > > many places as possible. It's kind of a gross > > kludge, and shouldn't really be > > needed if timeouts are correctly canceled, watchers > > are destroyed, etc. > > Perhaps it's time to get started on GObjectification! I agree. Is there any good reason to not start? I would even help writing patches if they are going to be accepted. Eduardo |
From: Mark D. <ma...@ki...> - 2006-09-18 16:40:12
|
On Mon, 18 Sep 2006 10:41:35 +0000, Eduardo Pérez wrote > On 2006-09-17 18:58:36 UTC, Adil wrote: > > --- Mark Doliner <ma...@ki...> wrote: > > > FYI I hope to make the use of > > > GAIM_CONNECTION_IS_VALID not necessary in as > > > many places as possible. It's kind of a gross > > > kludge, and shouldn't really be > > > needed if timeouts are correctly canceled, watchers > > > are destroyed, etc. > > > > Perhaps it's time to get started on GObjectification! > > I agree. > Is there any good reason to not start? > > I would even help writing patches if they are going to be accepted. I really really don't think we should start on this until after 2.0.0 is released. I'm against anything that would delay the release of 2.0.0. -Mark |
From: Mark H. <mar...@gm...> - 2006-09-22 15:36:10
|
On 9/17/06, Mark Doliner <ma...@ki...> wrote: > > FYI I hope to make the use of GAIM_CONNECTION_IS_VALID not necessary in as > many places as possible. It's kind of a gross kludge, and shouldn't > really be > needed if timeouts are correctly canceled, watchers are destroyed, etc. > > So ideally someone will figure out why that check is needed. When we disable our account via gaim_account_enable(), we destroy our GaimConnection. If we do this between the call to gaim_proxy_connect() and our prpl's got_login() callback, we will be passing that callback a pointer to freed memory. Hence the check in those functions. Therefore, if we don't find some alternative to GAIM_CONNECTION_IS_VALID, all of the prpls should be using it. What the checks KingAnt took out last night in the core were supposed to do, I have no idea. |
From: Mark D. <ma...@ki...> - 2006-09-22 17:23:45
|
On Fri, 22 Sep 2006 08:36:03 -0700, Mark Huetsch wrote > On 9/17/06, Mark Doliner <ma...@ki...> wrote: > > > > FYI I hope to make the use of GAIM_CONNECTION_IS_VALID not necessary in as > > many places as possible. It's kind of a gross kludge, and shouldn't > > really be > > needed if timeouts are correctly canceled, watchers are destroyed, etc. > > > > So ideally someone will figure out why that check is needed. > > When we disable our account via gaim_account_enable(), we destroy our > GaimConnection. If we do this between the call to > gaim_proxy_connect() and our prpl's got_login() callback, we will be > passing that callback a pointer to freed memory. Hence the check in > those functions. Therefore, if we don't find some alternative to > GAIM_CONNECTION_IS_VALID, all of the prpls should be using it. gaim_proxy_connect() returns a pointer to a GaimProxyConnectData. The alternative is to keep a reference to that struct (probably in gc), and cancel the connection using gaim_proxy_connect_cancel() if the account is destroyed before the connection attempt finishes. > What the checks KingAnt took out last night in the core were > supposed to do, I have no idea. The checks in the core were mostly unrelated. -Mark |
From: Eduardo <ep...@us...> - 2006-09-24 17:42:56
|
On 2006-09-22 17:28:20 UTC, Mark Doliner wrote: > On Fri, 22 Sep 2006 08:36:03 -0700, Mark Huetsch wrote > > On 9/17/06, Mark Doliner <ma...@ki...> wrote: > > > > > > FYI I hope to make the use of GAIM_CONNECTION_IS_VALID not necessary in as > > > many places as possible. It's kind of a gross kludge, and shouldn't > > > really be > > > needed if timeouts are correctly canceled, watchers are destroyed, etc. > > > > > > So ideally someone will figure out why that check is needed. > > > > When we disable our account via gaim_account_enable(), we destroy our > > GaimConnection. If we do this between the call to > > gaim_proxy_connect() and our prpl's got_login() callback, we will be > > passing that callback a pointer to freed memory. Hence the check in > > those functions. Therefore, if we don't find some alternative to > > GAIM_CONNECTION_IS_VALID, all of the prpls should be using it. > > gaim_proxy_connect() returns a pointer to a GaimProxyConnectData. The > alternative is to keep a reference to that struct (probably in gc), and cancel > the connection using gaim_proxy_connect_cancel() if the account is destroyed > before the connection attempt finishes. It seems like these problems would be solved by using GObject's reference counting, wouldn't it? |
From: Mark D. <ma...@ki...> - 2006-09-25 13:57:25
|
On Sun, 24 Sep 2006 17:41:53 +0000, Eduardo Pérez wrote > On 2006-09-22 17:28:20 UTC, Mark Doliner wrote: > > On Fri, 22 Sep 2006 08:36:03 -0700, Mark Huetsch wrote > > > On 9/17/06, Mark Doliner <ma...@ki...> wrote: > > > > > > > > FYI I hope to make the use of GAIM_CONNECTION_IS_VALID not necessary in as > > > > many places as possible. It's kind of a gross kludge, and shouldn't > > > > really be > > > > needed if timeouts are correctly canceled, watchers are destroyed, etc. > > > > > > > > So ideally someone will figure out why that check is needed. > > > > > > When we disable our account via gaim_account_enable(), we destroy our > > > GaimConnection. If we do this between the call to > > > gaim_proxy_connect() and our prpl's got_login() callback, we will be > > > passing that callback a pointer to freed memory. Hence the check in > > > those functions. Therefore, if we don't find some alternative to > > > GAIM_CONNECTION_IS_VALID, all of the prpls should be using it. > > > > gaim_proxy_connect() returns a pointer to a GaimProxyConnectData. The > > alternative is to keep a reference to that struct (probably in gc), and cancel > > the connection using gaim_proxy_connect_cancel() if the account is destroyed > > before the connection attempt finishes. > > It seems like these problems would be solved by using GObject's > reference counting, wouldn't it? They could be fixed by using GObject reference counting, yes. -Mark |
From: Evan S. <ev...@dr...> - 2006-09-25 14:15:08
Attachments:
PGP.sig
|
On Sep 25, 2006, at 10:02 AM, Mark Doliner wrote: >> It seems like these problems would be solved by using GObject's >> reference counting, wouldn't it? > > They could be fixed by using GObject reference counting, yes. Ideally, GObject reference counting would solve the potential crashes in question... but a cancellable system is definitely desireable. When a callback happens with proper reference counting but no cancelling, we wouldn't see a crash but instead potentially erratic behavior. -Evan |
From: Eduardo <ep...@us...> - 2006-09-25 15:42:39
|
On 2006-09-25 14:14:56 UTC, Evan Schoenberg wrote: > On Sep 25, 2006, at 10:02 AM, Mark Doliner wrote: > > >>It seems like these problems would be solved by using GObject's > >>reference counting, wouldn't it? > > > >They could be fixed by using GObject reference counting, yes. > > Ideally, GObject reference counting would solve the potential crashes > in question... but a cancellable system is definitely desireable. > When a callback happens with proper reference counting but no > cancelling, we wouldn't see a crash but instead potentially erratic > behavior. Of course a cancellable system is still needed. Reference counting is no substitute for a cancellable system, is it? Could you describe a case for that potentially erratic behavior? |
From: Evan S. <ev...@ad...> - 2006-09-26 15:15:47
|
On Sep 25, 2006, at 11:42 AM, Eduardo P=E9rez wrote: > Could you describe a case for that potentially erratic behavior? Anything which is set to execute on a timer or a delay or is the =20 result of a delayed callback (such as an asynchronous connection =20 request or DNS lookup) would lead to erratic behavior if it didn't =20 crash and was not cancelled, unless each possible callback for such a =20= situation does a GAIM_CONNECTION_IS_VALID() type check which we're =20 trying to move away from. -Evan |
From: Mark H. <mar...@gm...> - 2006-09-26 20:51:21
|
> > gaim_proxy_connect() returns a pointer to a GaimProxyConnectData. The > alternative is to keep a reference to that struct (probably in gc), and > cancel > the connection using gaim_proxy_connect_cancel() if the account is > destroyed > before the connection attempt finishes. Hmm, my solution would be to add a pointer to gc to the GaimProxyConnectData struct, initializing it appropriately in gaim_proxy_connect(). Then we could check in gaim_proxy_connect_data_connected() to ensure it hasn't been freed before calling the callback. Something like: static void gaim_proxy_connect_data_connected(GaimProxyConnectData *connect_data) { - connect_data->connect_cb(connect_data->data, connect_data->fd, NULL); + if (g_list_find(gaim_connections_get_all(), connect_data->gc)) + connect_data->connect_cb(connect_data->data, connect_data->fd, NULL); + else + g_free(connect_data->data) connect_data->fd = -1; gaim_proxy_connect_data_disconnect(connect_data, NULL); gaim_proxy_connect_data_destroy(connect_data); } We could do the same check in the callback for each asynchronous call to prevent some unnecessary function calls if the account was disabled early in the connection process, but that seems a bit excessive. It seems that this would also solve Evan's upload_buddy_icon() issue. |
From: Mark H. <mar...@gm...> - 2006-09-26 21:11:22
|
Oops, I don't think we want to free connect_data->data there, as it seems to always be our account or gc. We'd want to close(connect_data->fd) instead, moving "connect_data->fd = -1" into the successful branch. |
From: Mark D. <ma...@ki...> - 2006-09-26 23:08:55
|
On Tue, 26 Sep 2006 13:51:17 -0700, Mark Huetsch wrote > > > > gaim_proxy_connect() returns a pointer to a GaimProxyConnectData. The > > alternative is to keep a reference to that struct (probably in gc), and > > cancel > > the connection using gaim_proxy_connect_cancel() if the account is > > destroyed > > before the connection attempt finishes. > > Hmm, my solution would be to add a pointer to gc to the GaimProxyConnectData > struct, initializing it appropriately in gaim_proxy_connect(). Then > we could check in gaim_proxy_connect_data_connected() to ensure it > hasn't been freed before calling the callback. Something like: > > static void > gaim_proxy_connect_data_connected(GaimProxyConnectData *connect_data) > { > - connect_data->connect_cb(connect_data->data, connect_data- > >fd, NULL); + if (g_list_find(gaim_connections_get_all(), > connect_data->gc)) + connect_data- > >connect_cb(connect_data->data, connect_data->fd, NULL); + > else + g_free(connect_data->data) > > connect_data->fd = -1; > > gaim_proxy_connect_data_disconnect(connect_data, NULL); > gaim_proxy_connect_data_destroy(connect_data); > } > > We could do the same check in the callback for each asynchronous > call to prevent some unnecessary function calls if the account was > disabled early in the connection process, but that seems a bit excessive. > > It seems that this would also solve Evan's upload_buddy_icon() issue. Well for one thing gaim_proxy_connect() is not always used within a GaimConnection (for example, the version checker plugin calls gaim_util_fetch_url() which calls gaim_proxy_connect()). One reason I don't like the g_list_find()/GAIM_CONNECTION_IS_VALID() approach is that I think it's possible for one connection to be destroyed and another one created and they'll have the same location in memory and stuff will still crash. Although I'm sure that's extremely unlikely. It also means we have to iterate through the list of connections each time we make a connection. While the time required for this is pretty negligible compared to the tons of other stuff we do within Gaim, it seems silly do iterate through a linked list when we don't really need to. And of course, since we would no longer actually be canceling the connection then we might go through the whole connection process only to discover that we don't need the connection anymore, when in fact we could have just stopped after the IP address was resolved. Basically I just think that g_list_find()/GAIM_CONNECTION_IS_VALID() is an ugly programming practice. If we don't want stuff to have to keep track of GaimProxyConnectData then we can do the handle thing and add a gaim_proxy_connect_close_with_handle(gc) function. -Mark |
From: Evan S. <ev...@dr...> - 2006-09-26 21:04:06
Attachments:
PGP.sig
|
On Sep 26, 2006, at 4:51 PM, Mark Huetsch wrote: > Hmm, my solution would be to add a pointer to gc to the > GaimProxyConnectData struct, initializing it appropriately in > gaim_proxy_connect(). Then we could check in > gaim_proxy_connect_data_connected() to ensure it hasn't been freed > before calling the callback. Something like: > > static void > gaim_proxy_connect_data_connected(GaimProxyConnectData *connect_data) > { > - connect_data->connect_cb(connect_data->data, connect_data- > >fd, NULL); > + if (g_list_find(gaim_connections_get_all(), connect_data->gc)) > + connect_data->connect_cb(connect_data->data, > connect_data->fd, NULL); > + else > + g_free(connect_data->data) The g_list_find() call there is the same as GAIM_CONNECTION_IS_VALID () (a #define in connection.h) and a desire for it to be unnecessary is part of why kingant started the cancellable connections in the first place... however, that's a really interesting thought -- using it in exactly one place doesn't seem so bad when that means that any connection which doesn't get cancelled for a disconnected account will complete its connect process and then silently go away. This would provide a level of failsafe, while all existing code should still be converted to cancel their gaim_proxy_connect() attempts on disconnect... kingant, what do you think? -Evan |
From: Mark D. <ma...@ki...> - 2006-10-02 00:54:19
|
On Tue, 26 Sep 2006 17:03:44 -0400, Evan Schoenberg wrote > On Sep 26, 2006, at 4:51 PM, Mark Huetsch wrote: > > > Hmm, my solution would be to add a pointer to gc to the > > GaimProxyConnectData struct, initializing it appropriately in > > gaim_proxy_connect(). Then we could check in > > gaim_proxy_connect_data_connected() to ensure it hasn't been freed > > before calling the callback. Something like: > > > > static void > > gaim_proxy_connect_data_connected(GaimProxyConnectData *connect_data) > > { > > - connect_data->connect_cb(connect_data->data, connect_data- > > >fd, NULL); > > + if (g_list_find(gaim_connections_get_all(), connect_data->gc)) > > + connect_data->connect_cb(connect_data->data, > > connect_data->fd, NULL); > > + else > > + g_free(connect_data->data) > > The g_list_find() call there is the same as GAIM_CONNECTION_IS_VALID > > () (a #define in connection.h) and a desire for it to be unnecessary > is part of why kingant started the cancellable connections in the > first place... however, that's a really interesting thought -- > using it in exactly one place doesn't seem so bad when that means > that any connection which doesn't get cancelled for a disconnected > account will complete its connect process and then silently go > away. This would provide a level of failsafe, while all existing > code should still be converted to cancel their gaim_proxy_connect() > attempts on disconnect... kingant, what do you think? I don't really like that idea. It seems like it would involve changing the API to pass in a gc to gaim_proxy_connect(), and then at some point in the future we would want to remove that parameter. I'd rather not add something to the API that we'll have to remove later. -Mark |