Opened 10 years ago

Closed 10 years ago

#8758 closed defect (fixed)

perl plugin does not get unloaded

Reported by: cavedon Owned by: deryni
Milestone: Component: plugins
Version: 2.5.5 Keywords:
Cc:

Description

When purple_core_quit() is called the perl plugin is not unloaded. This happens because of the code (libpurple/plugins/perl/perl.c):

	/* Mostly evil hack... puts perl.so's symbols in the global table but
	 * does not create a circular dependency because g_module_open will
	 * only open the library once. */
	/* Do we need to keep track of the returned GModule here so that we
	 * can g_module_close it when this plugin gets unloaded?
	 * At the moment I don't think this plugin can ever get unloaded but
	 * in case that becomes possible this wants to get noted. */
	g_module_open("perl.so", 0);

Why is this line needed?

The problem with the perl plugin not being unloaded is not just a memory concern: if purple_core_init() is called again, static variables inside the perl plugin are not reinitialized, and keep the old values (i.e. pointer to no longer valid memory locations).

Attachments (1)

plugin-unload.patch (288 bytes) - added by cavedon 10 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 years ago by deryni

What sort of constants are causing you problems? The memory locations of the perl plugin should remain valid I would imagine as the module is never unloaded (the perl.so indicated there is the upstream perl shared object, that is the perl language library not pidgin's perl.so).do

However, as that comment indicates, it was clear that there may be reasons for pidgin to need to unload the module as some point. A patch which tracked the returned handle and unloaded it when the perl plugin is unloaded would be accepted.

I forget, offhand, what the need for that line was but I do recall it being necessary at the time something to do with not having the perl symbols available or something like that (potentially due to the way libpurple was loading plugins).

comment:2 Changed 10 years ago by deryni

  • Owner set to deryni

comment:3 Changed 10 years ago by cavedon

I see your point that the static pointers should be valid. However, when the purple_core_init() is called the second time, init_plugin() of the perl plugin is called again, and I noticed that, at that time, loader_info.exts contains an invalid address the use of which is causing memory corruption.

Looks like that purple_plugin_destroy() in plugin.c is destroying the list in loader_info.exts. That is the reason why the pointer is not valid.

A fix would be to always create a new list in perl's init_plugin(), as long as we are sure it is not possible for it to be called twice without calling purple_plugin_destroy() in between. Are we?

A patch which tracked the returned handle and unloaded it when the perl plugin is unloaded would be accepted.

I am not sure how you would do that: just call g_module_close() on return value of g_module_open() at module unload?

comment:4 follow-up: Changed 10 years ago by deryni

  • Status changed from new to pending

Ah, that seems entirely reasonable and likely. It would seem to me that the correct fix for this would be to NULLif the exts pointer when it is destroyed in purple_plugin_destroy. Are you able to test if that fixes your problem?

Adding

loader_info->exts = NULL;

immediately after the

g_list_free(loader_info->exts);

line in plugin.c should do it.

As to the perl.so loading, yes, it appears that simply saving the return value of g_module_open and giving it to g_module_close is the correct thing to do here. Though if the above fixes your issue, I'm inclined to leave this alone as it has not appeared to cause anyone any problems so far. Though if you want to test this as well in your envionment that would be fine and is likely the worst-case scenario for this change to break things.

comment:5 in reply to: ↑ 4 Changed 10 years ago by cavedon

  • Status changed from pending to new

Replying to deryni:

Ah, that seems entirely reasonable and likely. It would seem to me that the correct fix for this would be to NULLif the exts pointer when it is destroyed in purple_plugin_destroy. Are you able to test if that fixes your problem?

I agree. I tried and it fixes my problem, thanks!

As to the perl.so loading, yes, it appears that simply saving the return value of g_module_open and giving it to g_module_close is the correct thing to do here. Though if the above fixes your issue, I'm inclined to leave this alone as it has not appeared to cause anyone any problems so far. Though if you want to test this as well in your envionment that would be fine and is likely the worst-case scenario for this change to break things.

I agree about leaving it as is. Actually I tried it, but it did not seem to work (even if I did not put too much care, so I am not sure if I did something wrong).

comment:6 Changed 10 years ago by cavedon

Uhm, I did not meacn to change the status from pending to new! Looks like trac is doing this automatically? I see no option on my interface to set the pending status....

comment:7 Changed 10 years ago by deryni

The pending status means that we are waiting for an answer from the person who submitted the ticket, a ticket in pending status will be automatically closed after two weeks. A response will automatically set the status back to new so it doesn't close. No need to worry.

comment:8 Changed 10 years ago by cavedon

I guess I understood the reason why perl plugin cannot be unloaded: the tcl plugin has the same need: see #8770

comment:9 Changed 10 years ago by deryni

I don't believe the intention was to prevent perl.so from being unloaded (or the existing comment would be rather strange), I believe the issue was with perl symbols not being available to the perl plugin or some-such issue. I think the issue was that libpurple was loading plugins with G_MODULE_BIND_LOCAL and this was causing symbol problems with the perl.so symbols or something like that. I really don't remember.

comment:10 Changed 10 years ago by cavedon

"perl.so" is not the perl library, but the pidgin perl plugin itself. The perl library "libperl.so" is loaded automatically because the perl plugin is dynamically linked against it.

I believe that the perl plugin is incrementing its own reference counter: in this way g_module_close() called on plugin unload will decrement the reference counter but not unload the shared library (and the perl symbols with it).

I think you are right, that we need perl symbols available after plugin unload (like we need tcl symbols), but this is achieved by preventing the unloading of the plugin itself, I think.

comment:11 Changed 10 years ago by deryni

  • Status changed from new to pending

You are certainly correct that I had the module names incorrect, perl.so is in fact the pidgin perl plugin.

That being said, I believe I am nonetheless correct in my recollection that the problem was a symbol location issue and not a post-unload issue. Though I am at a loss as to explain what the problem might have been.

It should be simple enough to test whether that line is necessary for normal perl plugin functionality or not, simple comment it out and see what happens. Either you will run into problems after purple_core_quit is called or the perl plugin itself or plugins written in perl will fail. (The initial commit message for the hack appears to support my memory that it was a plugin loading issue and not an unloading issue, especially as pidgin never calls purple_core_quit except when it is exiting, which is why you are finding all these bugs since your usage is different.)

comment:12 Changed 10 years ago by cavedon

  • Status changed from pending to new

If I remove it the g_module_load() line I have no problem (but I did not test using a real perl plugin, I do not have one to run with qutecom). If it were a problem like for the tcl plugin, I would have no guarantee to actually see it happening (in fact the tcl.so segfault is not happening with pidgin, but with qutecom).

Anyway, I think the best solution is what you proposed in #comment:5

comment:13 Changed 10 years ago by bernmeister

Can this ticket be closed? Or should it be moved to something like Patches Welcome?

Changed 10 years ago by cavedon

comment:14 Changed 10 years ago by cavedon

Here is the patch implementing the solution beging discussed. I though the change was so straightforward that attaching a patch would have been an overkill :) Thanks!

comment:15 Changed 10 years ago by darkrain42

That particular change has already been made (I'm unsure if it is already in a release or if it will be fixed in 2.6.0); I'll leave it up to deryni whether to leave this open to address the issue of trying to correct the evil hack or not.

comment:16 Changed 10 years ago by deryni

  • Resolution set to fixed
  • Status changed from new to closed

Without knowing whether the issues that the hack was added to work around are fixed I'm not sure we can reasonably remove the hack. I'm also not sure the hack actually hurts anything so I'm not sure the time spent investigating would be worth it. So I'm going to close it.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!