Skip to content
This repository has been archived by the owner. 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

Projects
None yet
1 participant
@icinga-migration
Copy link
Member

commented Oct 28, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2012

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

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2012

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

  • Status changed from New to Closed
@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2014

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.
You can’t perform that action at this time.