10 May
2016
10 May
'16
5:48 p.m.
The column name is currently not free'd. Some db bankends copy this
data so that memory is leaked. Some store internal database pointers
and those shouldn't be free'd. One returns a pointer to a stack
variable which shouldn't be done.
The patch cleans up all db backends to copy the column name and frees
that column name as part of the database result cleanup function.
Author: Chris Double <doublec(a)silentcircle.com>
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/611
-- Commit Summary --
* Free database column name as part of database result cleanup
-- File Changes --
M lib/srdb1/db_res.c (3)
M modules/db_berkeley/km_bdb_res.c (9)
M modules/db_mongodb/mongodb_dbase.c (9)
M modules/db_mysql/km_res.c (9)
M modules/db_postgres/km_res.c (10)
M modules/db_text/dbt_api.c (9)
M modules/db_unixodbc/res.c (8)
M modules/xmlrpc/xmlrpc.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/611.patch
https://github.com/kamailio/kamailio/pull/611.diff
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611
11 May
11 May
8:08 a.m.
I don't really like this approach. The column names are not used inside the modules
that conenct to the database, is only sqlops that exposes them and can be accessed in the
configuration file. So I would not like to clone the column names if they are not
necessary, some db operations are done quite frequest (e.g., auth db).
You haven't named the modules with issues, just some generic description that some do
and some don't. I am not saying that there are not issues, but they need to be
identified properly and see what is the best fix for it. I manly use mysql module for
databases (and some bits of mongo).
I remember there was a similar patch back in August-September last year more or less
trying to address some issues with the Postgres module, but afaik there was a different
patch.
Anyhow, there can be indeed cases when the column name must be cloned. For this case I
would go with the approach done for db result values. A flag is kept for each to indicate
whether it has to be freed or not. That means adding a flags or similar field in db column
structure that will be set by each db module if they do cloning of the column name. I can
add this to the core and you can come with the patch for the module that really needs this
cloning.
Of course, comments about my suggestion and other proposals are welcome.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611#issuecomment-218440401
10:13 a.m.
@miconda Makes sense. No need to malloc/free if it isn't absolutely required for a
commonly used code path. I'll look over the patch and see if I can see a better
approach(using flags to signify malloced pointer). One of the larger concerns here was
that clang's address sanitizer was pointing out that at least one of the cases there
was a pointer to stack memory being returned, which could lead to bad things as that stack
frame was used by another thread.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611#issuecomment-218471897
3 Jun
3 Jun
3:24 a.m.
@quentusrex - is it ok to pursue the way with the flag for column name allocation
(following the patch referenced above)? If so, I will close this pull request.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611#issuecomment-223509456
8:51 a.m.
Closed #611.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611#event-681089393
8:51 a.m.
I'll update the patch and open a new PR when it's ready.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/611#issuecomment-223570443
3132
days inactive
3156
days old
5 comments
2 participants
participants (2)
-
Daniel-Constantin Mierla -
William King