sql_cacher: Fix possible invalid memory access

This patch ensures that we properly duplicate (and later free) any
strings contained in SQL DB results before freeing these results and
returning the strings to the calling layers.

We also add the is_str_column() macro, which improves code readability.

modules/sql_cacher/sql_cacher.c

@@ -1294,7 +1294,7 @@ static int cdb_val_decode(pv_name_fix_t *pv_name, str *cdb_val, int reload_versi
 		goto error;
 	memcpy(&int_val, int_buf, 4);
 
-	if ((pv_name->c_entry->column_types & (1LL << pv_name->col_nr)) != 0) {
+	if (is_str_column(pv_name)) {
 		/* null string value in db */
 		if (int_val == 0)
 			return 1;
@@ -1389,7 +1389,7 @@ static int on_demand_load(pv_name_fix_t *pv_name, str *str_res, int *int_res,
 							int rld_vers)
 {
 	struct queried_key *it, *tmp, *new_key;
-	str src_key;
+	str src_key, st;
 	str cdb_res;
 	db_res_t *sql_res = NULL;
 	db_val_t *values;
@@ -1525,14 +1525,27 @@ static int on_demand_load(pv_name_fix_t *pv_name, str *str_res, int *int_res,
 	val_type = VAL_TYPE(values + pv_name->col_nr);
 	switch (val_type) {
 		case DB_STRING:
-			str_res->s = (char *)VAL_STRING(values + pv_name->col_nr);
-			str_res->len = strlen(str_res->s);
+			st.s = (char *)VAL_STRING(values + pv_name->col_nr);
+			st.len = strlen(st.s);
+			if (pkg_str_dup(str_res, &st) != 0) {
+				LM_ERR("oom\n");
+				rc = -1;
+				goto out_free_res;
+			}
 			break;
 		case DB_STR:
-			str_res = &(VAL_STR(values + pv_name->col_nr));
+			if (pkg_str_dup(str_res, &(VAL_STR(values + pv_name->col_nr))) != 0) {
+				LM_ERR("oom\n");
+				rc = -1;
+				goto out_free_res;
+			}
 			break;
 		case DB_BLOB:
-			str_res = &(VAL_BLOB(values + pv_name->col_nr));
+			if (pkg_str_dup(str_res, &(VAL_BLOB(values + pv_name->col_nr))) != 0) {
+				LM_ERR("oom\n");
+				rc = -1;
+				goto out_free_res;
+			}
 			break;
 		case DB_INT:
 			*int_res = VAL_INT(values + pv_name->col_nr);
@@ -1650,7 +1663,7 @@ int pv_get_sql_cached_value(struct sip_msg *msg,  pv_param_t *param, pv_value_t
 	int rc, rc2, int_res = 0, l = 0;
 	char *ch = NULL;
 	str str_res = {NULL, 0}, cdb_res = {NULL, 0};
-	int entry_rld_vers;
+	int entry_rld_vers, free_str_res = 0;
 
 	if (!param || param->pvn.type != PV_NAME_PVAR ||
 		!param->pvn.u.dname) {
@@ -1736,6 +1749,8 @@ int pv_get_sql_cached_value(struct sip_msg *msg,  pv_param_t *param, pv_value_t
 				goto out_free_null;
 			} else if (rc2 != 0)
 				goto out_free_null;
+
+			free_str_res = 1;
 		} else {
 			if (cdb_res.len == 0 || !cdb_res.s) {
 				LM_DBG("key: %.*s not found in SQL db\n", pv_name->key.len, pv_name->key.s);
@@ -1760,19 +1775,26 @@ int pv_get_sql_cached_value(struct sip_msg *msg,  pv_param_t *param, pv_value_t
 					goto out_free_null;
 				} else if (rc2 != 0)
 					goto out_free_null;
+
+				free_str_res = 1;
 			}
 		}
 	}
 
-	if ((pv_name->c_entry->column_types & (1LL << pv_name->col_nr)) != 0) {
+	if (is_str_column(pv_name)) {
 		if (pkg_str_extend(&valbuff, str_res.len) != 0) {
 			LM_ERR("failed to alloc buffer\n");
+			if (free_str_res)
+				pkg_free(str_res.s);
 			goto out_free_null;
 		}
 
 		memcpy(valbuff.s, str_res.s, str_res.len);
 		valbuff.len = str_res.len;
 
+		if (free_str_res)
+			pkg_free(str_res.s);
+
 		res->flags = PV_VAL_STR;
 		res->rs.s = valbuff.s;
 		res->rs.len = str_res.len;

modules/sql_cacher/sql_cacher.h

@@ -59,6 +59,9 @@
 #define CDB_TEST_VAL_STR "sql_cacher_cdb_test_val"
 #define INT_B64_ENC_LEN 8
 
+#define is_str_column(pv_name_fix_p) \
+	((pv_name_fix_p)->c_entry->column_types & (1LL << (pv_name_fix_p)->col_nr))
+
 typedef struct _cache_entry {
 	str id;
 	str db_url;
@@ -105,4 +108,4 @@ typedef struct _pv_name_fix
 	char last_str;
 } pv_name_fix_t;
 
-#endif
+#endif