diff --git a/src/manage_acl.c b/src/manage_acl.c index ce7ca0abd..2829b6099 100644 --- a/src/manage_acl.c +++ b/src/manage_acl.c @@ -479,11 +479,18 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) * is "name" then "SELECT ... format" will choose arbitrarily between * the resources that have the same name. */ /** - * @brief Super clause. + * @brief Super clause with the resource value and the user UUID bound. * - * @param[in] format Value format specifier. + * The resource value becomes $1 and the user UUID $2. Both appear three + * times in the clause; PostgreSQL allows a parameter to be used more than + * once. + * + * The resource type, the trash suffix and the field name stay in the + * statement text. They come from gvmd's own tables of valid types, never + * from user input, and take only a handful of combinations, which keeps + * the statement cache in sql_pg.c small. */ -#define ACL_SUPER_CLAUSE(format) \ +#define ACL_SUPER_CLAUSE_PS \ " name = 'Super'" \ /* Super on everyone. */ \ " AND ((resource = 0)" \ @@ -491,7 +498,7 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) " OR ((resource_type = 'user')" \ " AND (resource = (SELECT %ss%s.owner" \ " FROM %ss%s" \ - " WHERE %s = " format ")))" \ + " WHERE %s = $1)))" \ /* Super on other_user's role. */ \ " OR ((resource_type = 'role')" \ " AND (resource" \ @@ -501,7 +508,7 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) " = (SELECT %ss%s.owner" \ " FROM %ss%s" \ " WHERE %s" \ - " = " format "))))" \ + " = $1))))" \ /* Super on other_user's group. */ \ " OR ((resource_type = 'group')" \ " AND (resource" \ @@ -510,12 +517,12 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) " WHERE \"user\"" \ " = (SELECT %ss%s.owner" \ " FROM %ss%s" \ - " WHERE %s = " format ")))))" \ + " WHERE %s = $1)))))" \ " AND subject_location = " G_STRINGIFY (LOCATION_TABLE) \ " AND ((subject_type = 'user'" \ " AND subject" \ " = (SELECT id FROM users" \ - " WHERE users.uuid = '%s'))" \ + " WHERE users.uuid = $2))" \ " OR (subject_type = 'group'" \ " AND subject" \ " IN (SELECT DISTINCT \"group\"" \ @@ -524,7 +531,7 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) " = (SELECT id" \ " FROM users" \ " WHERE users.uuid" \ - " = '%s')))" \ + " = $2)))" \ " OR (subject_type = 'role'" \ " AND subject" \ " IN (SELECT DISTINCT role" \ @@ -533,39 +540,34 @@ acl_user_has_role (const char *user_uuid, const char *role_uuid) " = (SELECT id" \ " FROM users" \ " WHERE users.uuid" \ - " = '%s'))))" + " = $2))))" /** - * @brief Super clause arguments. + * @brief Structural arguments for ACL_SUPER_CLAUSE_PS. * - * @param[in] type Type of resource. - * @param[in] field Field to compare. Typically "uuid". - * @param[in] value Expected value of field. - * @param[in] user_id UUID of user. - * @param[in] trash Whether to search trash. + * The value and the user UUID are not among them; they are bound as $1 + * and $2. + * + * @param[in] type Type of resource. + * @param[in] field Field to compare. Typically "uuid". + * @param[in] trash Whether to search trash. */ -#define ACL_SUPER_CLAUSE_ARGS(type, field, value, user_id, trash) \ +#define ACL_SUPER_CLAUSE_PS_ARGS(type, field, trash) \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ field, \ - value, \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ field, \ - value, \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ type, \ trash ? (strcasecmp (type, "task") ? "_trash" : "") : "", \ - field, \ - value, \ - user_id, \ - user_id, \ - user_id + field /** * @brief Test whether a user has Super permission on a resource. @@ -581,18 +583,24 @@ static int acl_user_has_super_on (const char *type, const char *field, const char *value, int trash) { - gchar *quoted_value; - quoted_value = sql_quote (value); - if (sql_int ("SELECT EXISTS (SELECT * FROM permissions" - " WHERE " ACL_SUPER_CLAUSE ("'%s'") ");", - ACL_SUPER_CLAUSE_ARGS (type, field, quoted_value, - current_credentials.uuid, trash))) - { - g_free (quoted_value); - return 1; - } - g_free (quoted_value); - return 0; + gchar *statement; + int ret; + + /* Value and user UUID are bound, so the statement text only varies with + * type, trash and field - see ACL_SUPER_CLAUSE_PS. A bound parameter is + * never part of the statement text, so there is nothing to escape. */ + statement = g_strdup_printf ("SELECT EXISTS (SELECT * FROM permissions" + " WHERE " ACL_SUPER_CLAUSE_PS + ");", + ACL_SUPER_CLAUSE_PS_ARGS (type, field, trash)); + + ret = sql_int_ps (statement, + SQL_STR_PARAM (value), + SQL_STR_PARAM (current_credentials.uuid), + NULL); + g_free (statement); + + return ret ? 1 : 0; } /** @@ -609,12 +617,24 @@ static int acl_user_has_super_on_resource (const char *type, const char *field, resource_t resource, int trash) { - if (sql_int ("SELECT EXISTS (SELECT * FROM permissions" - " WHERE " ACL_SUPER_CLAUSE ("%llu") ");", - ACL_SUPER_CLAUSE_ARGS (type, field, resource, - current_credentials.uuid, trash))) - return 1; - return 0; + gchar *statement; + int ret; + + /* As in acl_user_has_super_on, but the resource is numeric. The field + * name sits in the statement text, so this call site gets its own cache + * entry and PostgreSQL infers $1 as the column type of that field. */ + statement = g_strdup_printf ("SELECT EXISTS (SELECT * FROM permissions" + " WHERE " ACL_SUPER_CLAUSE_PS + ");", + ACL_SUPER_CLAUSE_PS_ARGS (type, field, trash)); + + ret = sql_int_ps (statement, + SQL_RESOURCE_PARAM (resource), + SQL_STR_PARAM (current_credentials.uuid), + NULL); + g_free (statement); + + return ret ? 1 : 0; } /** @@ -662,7 +682,6 @@ int acl_user_owns_uuid (const char *type, const char *uuid, int trash) { int ret; - gchar *quoted_uuid; assert (current_credentials.uuid); @@ -676,45 +695,55 @@ acl_user_owns_uuid (const char *type, const char *uuid, int trash) if (acl_user_has_super_on (type, "uuid", uuid, 0)) return 1; - quoted_uuid = sql_quote (uuid); - if (strcmp (type, "result") == 0) - ret = sql_int ("SELECT count(*) FROM results, reports" - " WHERE results.uuid = '%s'" - " AND results.report = reports.id" - " AND (reports.owner = (SELECT users.id FROM users" - " WHERE users.uuid = '%s'));", - quoted_uuid, - current_credentials.uuid); - else if (strcmp (type, "report") == 0) - ret = sql_int ("SELECT count(*) FROM reports" - " WHERE uuid = '%s'" - " AND (owner = (SELECT users.id FROM users" - " WHERE users.uuid = '%s'));", - quoted_uuid, - current_credentials.uuid); - else if (strcmp (type, "permission") == 0) - ret = sql_int ("SELECT count(*) FROM permissions%s" - " WHERE uuid = '%s'" - " AND ((owner IS NULL)" - " OR (owner = (SELECT users.id FROM users" - " WHERE users.uuid = '%s')));", - trash ? "_trash" : "", - quoted_uuid, - current_credentials.uuid); - else - ret = sql_int ("SELECT count(*) FROM %ss%s" - " WHERE uuid = '%s'" - "%s" - " AND (owner = (SELECT users.id FROM users" - " WHERE users.uuid = '%s'));", - type, - (strcmp (type, "task") && trash) ? "_trash" : "", - quoted_uuid, - (strcmp (type, "task") - ? "" - : (trash ? " AND hidden = 2" : " AND hidden < 2")), - current_credentials.uuid); - g_free (quoted_uuid); + /* type is not user input; it comes from gvmd's own resource type names, + * and the callers that reach this point have passed + * valid_db_resource_type. The UUIDs are bound as $1 and $2, so they are + * not part of the statement text and need no quoting. */ + { + gchar *statement; + + if (strcmp (type, "result") == 0) + statement + = g_strdup ("SELECT count(*) FROM results, reports" + " WHERE results.uuid = $1" + " AND results.report = reports.id" + " AND (reports.owner = (SELECT users.id FROM users" + " WHERE users.uuid = $2));"); + else if (strcmp (type, "report") == 0) + statement + = g_strdup ("SELECT count(*) FROM reports" + " WHERE uuid = $1" + " AND (owner = (SELECT users.id FROM users" + " WHERE users.uuid = $2));"); + else if (strcmp (type, "permission") == 0) + statement + = g_strdup_printf ("SELECT count(*) FROM permissions%s" + " WHERE uuid = $1" + " AND ((owner IS NULL)" + " OR (owner = (SELECT users.id FROM users" + " WHERE users.uuid = $2)));", + trash ? "_trash" : ""); + else + statement + = g_strdup_printf ("SELECT count(*) FROM %ss%s" + " WHERE uuid = $1" + "%s" + " AND (owner = (SELECT users.id FROM users" + " WHERE users.uuid = $2));", + type, + (strcmp (type, "task") && trash) ? "_trash" : "", + (strcmp (type, "task") + ? "" + : (trash + ? " AND hidden = 2" + : " AND hidden < 2"))); + + ret = sql_int_ps (statement, + SQL_STR_PARAM (uuid), + SQL_STR_PARAM (current_credentials.uuid), + NULL); + g_free (statement); + } return ret; } diff --git a/src/manage_sql_resources.c b/src/manage_sql_resources.c index f8adfc4bd..ac66e494c 100644 --- a/src/manage_sql_resources.c +++ b/src/manage_sql_resources.c @@ -325,40 +325,53 @@ find_resource_with_permission (const char* type, const char* uuid, *resource = 0; return FALSE; } - switch (sql_int64 (resource, - "SELECT id FROM %ss%s WHERE uuid = '%s'%s%s;", - type, - (trash && strcmp (type, "task") && strcmp (type, "report")) - ? "_trash" - : "", - quoted_uuid, - strcmp (type, "task") - ? "" - : (trash ? " AND hidden = 2" : " AND hidden < 2"), - strcmp (type, "report") - ? "" - : (trash - ? " AND (SELECT hidden FROM tasks" - " WHERE tasks.id = task)" - " = 2" - : " AND (SELECT hidden FROM tasks" - " WHERE tasks.id = task)" - " = 0"))) - { - case 0: - break; - case 1: /* Too few rows in result of query. */ - *resource = 0; - break; - default: /* Programming error. */ - assert (0); - case -1: - g_free (quoted_uuid); - return TRUE; - break; - } - g_free (quoted_uuid); + /* The UUID is bound, so the statement text only varies with type and + * trash - a handful of combinations, which the statement cache in + * sql_pg.c can keep plans for. type is safe to keep in the text: + * valid_db_resource_type above rejects anything that is not a known + * resource type, so it never carries user input. */ + { + gchar *statement; + + statement + = g_strdup_printf ("SELECT id FROM %ss%s WHERE uuid = $1%s%s;", + type, + (trash && strcmp (type, "task") + && strcmp (type, "report")) + ? "_trash" + : "", + strcmp (type, "task") + ? "" + : (trash ? " AND hidden = 2" : " AND hidden < 2"), + strcmp (type, "report") + ? "" + : (trash + ? " AND (SELECT hidden FROM tasks" + " WHERE tasks.id = task)" + " = 2" + : " AND (SELECT hidden FROM tasks" + " WHERE tasks.id = task)" + " = 0")); + + switch (sql_int64_ps (resource, statement, SQL_STR_PARAM (uuid), NULL)) + { + case 0: + break; + case 1: /* Too few rows in result of query. */ + *resource = 0; + break; + default: /* Programming error. */ + assert (0); + case -1: + g_free (statement); + return TRUE; + break; + } + + g_free (statement); + } + return FALSE; } diff --git a/src/manage_sql_tags.c b/src/manage_sql_tags.c index 0485789b7..a9ce7f181 100644 --- a/src/manage_sql_tags.c +++ b/src/manage_sql_tags.c @@ -1374,21 +1374,33 @@ resource_tag_count (const char* type, resource_t resource, int active_only) else parent_type = type; - ret = sql_int ("SELECT count (id)" - " FROM tags" - " WHERE resource_type = '%s'" - " AND EXISTS" - " (SELECT * FROM tag_resources" - " WHERE tag = tags.id" - " AND resource = %llu" - " AND resource_location = %d" - " AND tag_resources.resource_type = '%s')" - " %s;", - type, - resource, - LOCATION_TABLE, - parent_type, - active_only ? "AND active=1": ""); + /* Everything except the active_only clause is bound, so this has + * exactly two statement texts and the cache in sql_pg.c keeps a plan + * for each. */ + { + gchar *statement; + + statement + = g_strdup_printf ("SELECT count (id)" + " FROM tags" + " WHERE resource_type = $1" + " AND EXISTS" + " (SELECT * FROM tag_resources" + " WHERE tag = tags.id" + " AND resource = $2" + " AND resource_location = " + G_STRINGIFY (LOCATION_TABLE) + " AND tag_resources.resource_type = $3)" + " %s;", + active_only ? "AND active=1": ""); + + ret = sql_int_ps (statement, + SQL_STR_PARAM (type), + SQL_RESOURCE_PARAM (resource), + SQL_STR_PARAM (parent_type), + NULL); + g_free (statement); + } return ret; } diff --git a/src/manage_sql_targets.c b/src/manage_sql_targets.c index c896dcb26..3b1b0d603 100644 --- a/src/manage_sql_targets.c +++ b/src/manage_sql_targets.c @@ -2155,11 +2155,12 @@ target_task_iterator_readable (iterator_t* iterator) int target_in_use (target_t target) { - return !!sql_int ("SELECT count(*) FROM tasks" - " WHERE target = %llu" - " AND target_location = " G_STRINGIFY (LOCATION_TABLE) - " AND hidden = 0;", - target); + return !!sql_int_ps ("SELECT count(*) FROM tasks" + " WHERE target = $1" + " AND target_location = " G_STRINGIFY (LOCATION_TABLE) + " AND hidden = 0;", + SQL_RESOURCE_PARAM (target), + NULL); } /** diff --git a/src/sql_pg.c b/src/sql_pg.c index 6b248b93c..8412807b1 100644 --- a/src/sql_pg.c +++ b/src/sql_pg.c @@ -64,6 +64,50 @@ extern int log_errors; */ static PGconn *conn = NULL; +/** + * @brief Server side prepared statements, SQL text to statement name. + * + * Without this every statement is parsed and planned again on each + * call: PQexecParams sends the statement text every time and PostgreSQL + * plans it every time. Only PQprepare plus PQexecPrepared lets the + * server keep the plan. + * + * Keyed by the statement text, which is a compile time constant at each + * of the call sites that pass parameters, so the table stays bounded by + * the number of such call sites. + * + * Only statements that actually bind parameters are cached. The printf + * style calls format their values straight into the text, so every call + * would produce a different key and the table would grow without limit - + * and there would be nothing to gain, because a plan for + * "WHERE id = 4711" is of no use to "WHERE id = 4712". */ +static GHashTable *prepared_statements = NULL; + +/** + * @brief Counter for generating prepared statement names. + */ +static unsigned int prepared_statement_count = 0; + +/** + * @brief Forget all prepared statements. + * + * Prepared statements live in the database session. Whenever the connection + * goes away the names become meaningless, and reusing one would make the + * next PQexecPrepared fail with "prepared statement does not exist". So this + * has to run everywhere conn is dropped, including sql_close_fork, where the + * child abandons the parent's connection without closing it. + */ +static void +sql_prepared_statements_clear () +{ + if (prepared_statements) + { + g_hash_table_destroy (prepared_statements); + prepared_statements = NULL; + } + prepared_statement_count = 0; +} + /* Helpers. */ @@ -378,6 +422,7 @@ sql_open (const db_conn_info_t *database) fail: PQfinish (conn); conn = NULL; + sql_prepared_statements_clear (); semaphore_op (SEMAPHORE_DB_CONNECTIONS, +1, 0); return -1; } @@ -390,6 +435,7 @@ sql_close () { PQfinish (conn); conn = NULL; + sql_prepared_statements_clear (); semaphore_op (SEMAPHORE_DB_CONNECTIONS, +1, 0); } @@ -400,6 +446,7 @@ void sql_close_fork () { conn = NULL; + sql_prepared_statements_clear (); } /** @@ -526,6 +573,60 @@ sql_prepare_ps_internal (int log, const char *sql, va_list args, va_end (args_copy); } +/** + * @brief Get the name of the server side prepared statement for a statement. + * + * Prepares it on first use. Statements without parameters are not cached, + * see the comment on prepared_statements. + * + * A failed PQprepare is not treated as an error: the caller falls back to + * PQexecParams, which reports the problem in the usual place with the usual + * message. Failing here instead would turn every ordinary SQL error into a + * second, more confusing one. + * + * @param[in] stmt Statement. + * + * @return Statement name, or NULL to execute the statement directly. + */ +static const char * +sql_prepared_statement_name (sql_stmt_t *stmt) +{ + gchar *name; + PGresult *result; + + /* Nothing bound means the values are already in the text. See the comment + * on prepared_statements for why caching those would be pointless. */ + if (stmt->param_values->len == 0) + return NULL; + + if (prepared_statements == NULL) + prepared_statements = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, g_free); + + name = g_hash_table_lookup (prepared_statements, stmt->sql); + if (name) + return name; + + name = g_strdup_printf ("gvmd_ps_%u", prepared_statement_count); + + result = PQprepare (conn, name, stmt->sql, stmt->param_values->len, + NULL); /* Default param types. */ + if (PQresultStatus (result) != PGRES_COMMAND_OK) + { + g_debug ("%s: PQprepare failed, running statement directly: %s", + __func__, PQresultErrorMessage (result)); + g_debug ("%s: SQL: %s", __func__, stmt->sql); + PQclear (result); + g_free (name); + return NULL; + } + PQclear (result); + + prepared_statement_count++; + g_hash_table_insert (prepared_statements, g_strdup (stmt->sql), name); + return name; +} + /** * @brief Execute a statement. * @@ -543,12 +644,25 @@ sql_exec_internal (sql_stmt_t *stmt) if (stmt->executed == 0) { - result = PQexecParams (conn, stmt->sql, stmt->param_values->len, - NULL, /* Default param types. */ - (const char *const *) stmt->param_values->pdata, - (const int *) stmt->param_lengths->data, - (const int *) stmt->param_formats->data, - 0); /* Results as text. */ + const char *statement_name; + + statement_name = sql_prepared_statement_name (stmt); + + if (statement_name) + result = PQexecPrepared (conn, statement_name, + stmt->param_values->len, + (const char *const *) + stmt->param_values->pdata, + (const int *) stmt->param_lengths->data, + (const int *) stmt->param_formats->data, + 0); /* Results as text. */ + else + result = PQexecParams (conn, stmt->sql, stmt->param_values->len, + NULL, /* Default param types. */ + (const char *const *) stmt->param_values->pdata, + (const int *) stmt->param_lengths->data, + (const int *) stmt->param_formats->data, + 0); /* Results as text. */ ExecStatusType status = PQresultStatus (result); if (status != PGRES_TUPLES_OK && status != PGRES_COMMAND_OK