[Asterisk-code-review] func odbc: single database connection should be optional (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: func_odbc: single database connection should be optional
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/2800/3/funcs/func_odbc.c
File funcs/func_odbc.c:

Line 349: 	if (single_db_connection) {
Remove the check of single_db_connection from here.

Imagine that Asterisk is configured to have single_db_connection true. Then a DB query starts. Then someone issues a reload, setting single_db_connection false. Then the DB query completes.

At the beginning of the query, since single_db_connection was true, we retrieved a reference to a DSN and locked it. Now that the query is complete, we must unlock the DSN and unreference it. Unfortunately, now since single_db_connection is no longer true, we will instead leave the DSN locked, leak a reference to it, and potentially end up releasing that DSN's connection out from under it, possibly causing other queries in flight to cause Asterisk to crash.

By removing the check of single_db_connection, you should end up always performing the proper steps here.


Line 1756: 		ast_log(LOG_NOTICE, "Single database connection per DSN\n");
This looks like some debugging that you probably meant to remove.


Line 1850: 		ast_log(LOG_NOTICE, "Single database connection per DSN\n");
More extra debugging.


PS3, Line 1833: 
              : 	if (single_db_connection) {
              : 		ao2_ref(dsns, -1);
              : 	}
              : 
              : 	if (cfg && (s = ast_variable_retrieve(cfg, "general", "single_db_connection"))) {
              : 		single_db_connection = ast_true(s);
              : 	} else {
              : 		single_db_connection = DEFAULT_SINGLE_DB_CONNECTION;
              : 	}
              : 
              : 	if (single_db_connection) {
              : 		dsns = ao2_container_alloc(DSN_BUCKETS, dsn_hash, dsn_cmp);
              : 		if (!dsns) {
              : 			ast_log(LOG_ERROR, "Could not initialize DSN container\n");
              : 			return 0;
              : 		}
              : 		ast_log(LOG_NOTICE, "Single database connection per DSN\n");
              : 	}
This code is structured cleanly, but on a reload there are chances for race conditions.

Specifically, consider what happens if single_db_connection is true, and a query starts between when you have unreferenced the dsns container and re-allocated it? The query will attempt to look up a DSN in a freed container, and it's likely that Asterisk will crash.

The code needs to be segmented based on the change in value of single_db_connection.

* If it was previously true and the reload keeps it true, then do not make any changes to the dsns container at all.
* If it was previously false and is still false, then similarly, don't do anything with the dsns container.

Those are the easy ones. The harder ones are when single_db_connection changes values. You could minimize race conditions by ordering your operations on single_db_connection and the dsns container, but even then, since the operation is not atomic, there is the ever so slight chance for race conditions. Two ideas for avoiding the race conditions are:

* Switch res_odbc configuration to use the API provided by config_options.h.
* Protect access to the single_db_connection and dsns container with a lock (probably a read/write lock)

The first option would be hella cool, but I think the second option is more reasonable.

The other, lazier option would be to just not allow single_db_connection to change on reload. I'd only suggest doing that as a last resort though.


-- 
To view, visit https://gerrit.asterisk.org/2800
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d990616c957dabf7597dea5d5c3148f459dfb6
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-HasComments: Yes