Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

[dev.icinga.com #3405] Failure to free() libdbi results causes memory leaks in ido2db #1150

Closed
icinga-migration opened this issue Oct 28, 2012 · 11 comments

Comments

@icinga-migration
Copy link

This issue has been migrated from Redmine: https://dev.icinga.com/issues/3405

Created by crfriend on 2012-10-28 23:29:35 +00:00

Assignee: (none)
Status: Closed (closed on 2012-11-09 17:44:02 +00:00)
Target Version: (none)
Last Update: 2014-12-08 14:37:55 +00:00 (in Redmine)

Icinga Version: 1.10.0
OS Version: any

The changes required to get around the new "unsafeness" of "UPDATE .. ON DUPLICATE KEY" in MySQL resulted in a failure to properly free() the results from queries to the MySQL database. This, in turn, resulted in a memory leak. The attached patch, having been run for 3+ hours with no indicated leakage, attempts to address the issue.

It is worth noting that similar countermeasures may be required for Postgres databases, and the tactic used in this patch may well be applicable to Postgres. I was unable to test or verify that, however, as I do not have a Postgres database operational in my lab.

Attachments

Changesets

2012-10-29 18:31:20 +00:00 by mfriedrich 59bec47

idoutils: fix many memory leaks in ido2db on dbi_result_free and others (thx Klaus Wagner) refs #3406 #3405

Relations:

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-10-29 18:30:12 +00:00

thanks for the patch - can you double check that with #3406 as well as my git commit please?

@icinga-migration
Copy link
Author

Updated by crfriend on 2012-10-29 20:09:54 +00:00

dnsmichi wrote:

thanks for the patch - can you double check that with #3406 as well as my git commit please?

The observed behaviours in #3406 are the same as I was trying to fix. I suspect either patch-set will fix them.

In my case, I've been watching my patched version of ido2db for the past 24 hours and it's not leaked even a single page of mainstore.

At first blush, I believe that the same fixes are applicable to the Postgres database as well, but as I do not have an operational Postgres databsase here I opted to not try patching that and defer to someone who uses Postgres.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-10-29 20:14:21 +00:00

patch runs against mysql currently, but will switch once this is fully solved. see #3408 which is still needed.

@icinga-migration
Copy link
Author

Updated by crfriend on 2012-10-30 20:49:46 +00:00

My proposed patch above works well at home but leaks elsewhere on my systems at work which have a different load profile. {sigh} At least some of the leaks are plugged.

Am investigating #3408 and #3406 as I find time.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-10-30 21:19:09 +00:00

your patch does add a free sometimes after the second query was issued - which is then cleaned outside the call of the insert_or_update query. though, to note, within #3408 this must be fixed anyways.

@icinga-migration
Copy link
Author

Updated by crfriend on 2012-10-30 21:48:16 +00:00

Guidance request.

In looking at the reasons for leakage on my work systems it came to my attention that I had not updated the MySQL database schema correctly which caused some queries to fail on "Unknown column" -- and when queries failed memory allocated to create the query was not freed.

For the sake of completeness and correctness I feel this should be corrected with a patch. Does the community feel the same way? If so, I'll provide what I can find.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-10-30 22:13:53 +00:00

the query string should be free'd after each query call, no matter if the result was succesful or not. but if you find any leaking in that direction, of course, much appreciated!

@icinga-migration
Copy link
Author

Updated by crfriend on 2012-10-31 20:40:50 +00:00

  • File added 3405-query-fail-free.txt

dnsmichi wrote:

the query string should be free'd after each query call, no matter if the result was succesful or not. but if you find any leaking in that direction, of course, much appreciated!

It turns out that it wasn't a failure to free the query, it was part of the stuff used to generate the query that wasn't getting free()d. In any event, the only leakage I detected was from a single subroutine, and the patch called 3405-query-fail-free.txt is designed to address that one condition.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-10-31 21:14:20 +00:00

thanks for the finding, but that was already resolved by the other issue, as to be seen on dev/ido git branch currently.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2012-11-09 17:44:02 +00:00

  • Status changed from New to Closed

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-12-08 14:37:55 +00:00

  • Project changed from 18 to Core, Classic UI, IDOUtils
  • Category changed from 79 to IDOUtils
  • Icinga Version changed from 1 to 1
  • OS Version set to any

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant