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_pg.c b/src/manage_pg.c index 0c2e7e3ec..1902cfafb 100644 --- a/src/manage_pg.c +++ b/src/manage_pg.c @@ -43,10 +43,24 @@ check_db_extensions (); void manage_session_init (const char *uuid) { + /* Three statements - the lookup and the two SET SESSION - and the callers + * repeat them constantly on the same user. Creating an override switches + * user once per report and then straight back again, so over 1064 reports + * this ran about eleven times per report and nearly every call set the + * session to the user it was already on. + * + * The cache lives in sql_pg.c so that it is dropped whenever the + * connection or the transaction that carries the session state goes away. + * The two direct writers of these variables in manage_sql.c forget it by + * hand. */ + if (uuid && sql_session_uuid () && strcmp (sql_session_uuid (), uuid) == 0) + return; + sql ("SET SESSION \"gvmd.user.id\" = %llu;", sql_int64_0 ("SELECT id FROM users WHERE uuid = '%s';", uuid)); sql ("SET SESSION \"gvmd.tz_override\" = '';"); + sql_session_uuid_set (uuid); } /** @@ -3666,6 +3680,19 @@ create_tables () create_indexes_nvt (); + /* Rebuilding the count cache after an override changes runs two queries + * per affected report that select from overrides by NVT: the valid_overrides + * clause of the result iterator matches on result_nvt, and the end_time of + * the cache row is the earliest expiry among the overrides that touch the + * report, matched on nvt. Without these the table is scanned end to end + * every time, so creating an override got slower the more overrides were + * already stored - measured over 133 reports it went from 0.854 s at none + * to 0.992 s at 255. */ + sql ("SELECT create_index ('overrides_by_nvt'," + " 'overrides', 'nvt');"); + sql ("SELECT create_index ('overrides_by_result_nvt'," + " 'overrides', 'result_nvt');"); + sql ("SELECT create_index ('permissions_by_name'," " 'permissions', 'name');"); sql ("SELECT create_index ('permissions_by_resource'," @@ -4416,6 +4443,38 @@ manage_db_init_indexes (const gchar *name) return 0; } +/** + * @brief Check whether a table exists, by way of the system catalogue. + * + * These three predicates sit in hot paths - init_result_get_iterator_severity + * asks for CERT every time it builds a result iterator, which while an + * override is being created happens twice for every report the overridden NVT + * appears in. + * + * information_schema.tables is a view over pg_class, pg_namespace and the + * privilege functions, and gvmd formats the schema and table names straight + * into the statement text, so it is not one of the statements that reach the + * prepared statement cache and PostgreSQL plans the whole view again on every + * call. Measured on this database: + * + * information_schema planning 1.503 ms execution 0.215 ms (21 plan rows) + * to_regclass planning 0.035 ms execution 0.067 ms (3 plan rows) + * + * to_regclass is a syscache lookup and answers the same question. It returns + * NULL rather than raising when the object is absent, and an unknown schema is + * NULL too, so it needs no separate check that the schema exists. + * + * @param[in] qualified_name Schema qualified table name. + * + * @return 1 if the table exists, else 0. + */ +static int +table_exists_regclass (const char *qualified_name) +{ + return !!sql_int ("SELECT (to_regclass ('%s') IS NOT NULL)::integer;", + qualified_name); +} + /** * @brief Check whether CERT is available. * @@ -4424,12 +4483,7 @@ manage_db_init_indexes (const gchar *name) int manage_cert_loaded () { - return !!sql_int ("SELECT EXISTS (SELECT * FROM information_schema.tables" - " WHERE table_catalog = '%s'" - " AND table_schema = 'cert'" - " AND table_name = 'dfn_cert_advs')" - " ::integer;", - sql_database ()); + return table_exists_regclass ("cert.dfn_cert_advs"); } /** @@ -4440,12 +4494,7 @@ manage_cert_loaded () int manage_scap_loaded () { - return !!sql_int ("SELECT EXISTS (SELECT * FROM information_schema.tables" - " WHERE table_catalog = '%s'" - " AND table_schema = 'scap'" - " AND table_name = 'cves')" - " ::integer;", - sql_database ()); + return table_exists_regclass ("scap.cves"); } /** @@ -4456,12 +4505,7 @@ manage_scap_loaded () int manage_nvts_loaded () { - return !!sql_int ("SELECT EXISTS (SELECT * FROM information_schema.tables" - " WHERE table_catalog = '%s'" - " AND table_schema = 'public'" - " AND table_name = 'nvts')" - " ::integer;", - sql_database ()); + return table_exists_regclass ("public.nvts"); } /** diff --git a/src/manage_sql.c b/src/manage_sql.c index e8fe7b576..2c76cecf1 100644 --- a/src/manage_sql.c +++ b/src/manage_sql.c @@ -738,6 +738,13 @@ column_array_set (column_t *columns, const gchar *filter, gchar *select) * @param[in] acl_with_optional Whether default permission WITH clauses are * optional. * @param[in] assume_permitted Whether to skip permission checks. + * @param[in] bind_param Single value to bind as $1, or NULL to format + * every value into the statement text as usual. + * The caller is responsible for putting $1 into + * the fragments it passes in; see + * init_result_get_iterator_severity, which binds + * the report so that the same plan serves every + * report instead of one plan per report. * * @return 0 success, 1 failed to find resource, 2 failed to find filter, -1 * error. @@ -756,7 +763,8 @@ init_get_iterator2_with (iterator_t* iterator, const char *type, const char *extra_order, const char *extra_with, int acl_with_optional, - int assume_permitted) + int assume_permitted, + const sql_param_t *bind_param) { int first, max; gchar *clause, *order, *filter, *owned_clause, *with_clause; @@ -899,81 +907,98 @@ init_get_iterator2_with (iterator_t* iterator, const char *type, order = NULL; } - if (resource && get->trash) - init_iterator (iterator, - "%sSELECT %s" - " FROM %ss%s %s" - " WHERE %ss%s.id = %llu" - " AND %s%s" - "%s%s;", - with_clause ? with_clause : "", - columns, - type, - type_trash_in_table (type) ? "" : "_trash", - extra_tables ? extra_tables : "", - type, - type_trash_in_table (type) ? "" : "_trash", - resource, - owned_clause, - extra_where_single ? extra_where_single : "", - order ? order : "", - order ? (extra_order ? extra_order : "") : ""); - else if (get->trash) - init_iterator (iterator, - "%sSELECT %s" - " FROM %ss%s %s" - " WHERE" - "%s" - "%s" - "%s%s;", - with_clause ? with_clause : "", - columns, - type, - type_trash_in_table (type) ? "" : "_trash", - extra_tables ? extra_tables : "", - owned_clause, - extra_where ? extra_where : "", - order ? order : "", - order ? (extra_order ? extra_order : "") : ""); - else if (resource) - init_iterator (iterator, - "%sSELECT %s" - " FROM %ss %s" - " WHERE %ss.id = %llu" - " AND %s%s" - "%s%s;", - with_clause ? with_clause : "", - columns, - type, - extra_tables ? extra_tables : "", - type, - resource, - owned_clause, - extra_where_single ? extra_where_single : "", - order ? order : "", - order ? (extra_order ? extra_order : "") : ""); - else - init_iterator (iterator, - "%s%sSELECT %s" - " FROM %ss %s" - " WHERE" - " %s%s%s%s%s%s%s" - " LIMIT %s OFFSET %i%s;", - with_clause ? with_clause : "", - distinct ? "SELECT DISTINCT * FROM (" : "", - columns, - type, - extra_tables ? extra_tables : "", - owned_clause, - clause ? " AND (" : "", - clause ? clause : "", - clause ? ")" : "", - extra_where ? extra_where : "", - order ? order : "", - order ? (extra_order ? extra_order : "") : "", - sql_select_limit (max), - first, - distinct ? ") AS subquery_for_distinct" : ""); + /* Assemble the statement first and run it in one place, so that the bound + * form and the formatted form cannot drift apart. init_ps_iterator does + * not printf its argument - it takes the text as given - so the statement + * has to be complete before it goes in either way. */ + { + gchar *statement; + + if (resource && get->trash) + statement = g_strdup_printf + ("%sSELECT %s" + " FROM %ss%s %s" + " WHERE %ss%s.id = %llu" + " AND %s%s" + "%s%s;", + with_clause ? with_clause : "", + columns, + type, + type_trash_in_table (type) ? "" : "_trash", + extra_tables ? extra_tables : "", + type, + type_trash_in_table (type) ? "" : "_trash", + resource, + owned_clause, + extra_where_single ? extra_where_single : "", + order ? order : "", + order ? (extra_order ? extra_order : "") : ""); + else if (get->trash) + statement = g_strdup_printf + ("%sSELECT %s" + " FROM %ss%s %s" + " WHERE" + "%s" + "%s" + "%s%s;", + with_clause ? with_clause : "", + columns, + type, + type_trash_in_table (type) ? "" : "_trash", + extra_tables ? extra_tables : "", + owned_clause, + extra_where ? extra_where : "", + order ? order : "", + order ? (extra_order ? extra_order : "") : ""); + else if (resource) + statement = g_strdup_printf + ("%sSELECT %s" + " FROM %ss %s" + " WHERE %ss.id = %llu" + " AND %s%s" + "%s%s;", + with_clause ? with_clause : "", + columns, + type, + extra_tables ? extra_tables : "", + type, + resource, + owned_clause, + extra_where_single ? extra_where_single : "", + order ? order : "", + order ? (extra_order ? extra_order : "") : ""); + else + statement = g_strdup_printf + ("%s%sSELECT %s" + " FROM %ss %s" + " WHERE" + " %s%s%s%s%s%s%s" + " LIMIT %s OFFSET %i%s;", + with_clause ? with_clause : "", + distinct ? "SELECT DISTINCT * FROM (" : "", + columns, + type, + extra_tables ? extra_tables : "", + owned_clause, + clause ? " AND (" : "", + clause ? clause : "", + clause ? ")" : "", + extra_where ? extra_where : "", + order ? order : "", + order ? (extra_order ? extra_order : "") : "", + sql_select_limit (max), + first, + distinct ? ") AS subquery_for_distinct" : ""); + + if (bind_param) + init_ps_iterator (iterator, statement, bind_param, NULL); + else + /* "%s" and not the statement itself: the text is finished and must not + * be run through printf a second time. */ + init_iterator (iterator, "%s", statement); + + g_free (statement); + } g_free (columns); g_free (with_clause); @@ -1027,7 +1052,8 @@ init_get_iterator2 (iterator_t* iterator, const char *type, trash_select_columns, where_columns, trash_where_columns, filter_columns, distinct, extra_tables, extra_where, extra_where_single, - owned, ignore_id, extra_order, NULL, 0, 0); + owned, ignore_id, extra_order, NULL, 0, 0, + NULL); } /** @@ -3261,6 +3287,8 @@ init_manage_open_db_no_abort (const db_conn_info_t *database) /* Ensure the user session variables always exists. */ sql ("SET SESSION \"gvmd.user.id\" = 0;"); sql ("SET SESSION \"gvmd.tz_override\" = '';"); + /* Written here without going through manage_session_init. */ + sql_session_uuid_set (NULL); /* Attach the SCAP and CERT databases. */ manage_attach_databases (); @@ -5011,6 +5039,7 @@ manage_reset_currents () current_scanner_task = (task_t) 0; sql ("RESET \"gvmd.user.id\";"); sql ("RESET \"gvmd.tz_override\";"); + sql_session_uuid_set (NULL); free_credentials (¤t_credentials); } @@ -9010,6 +9039,19 @@ report_cache_counts (report_t report, int clear_original, int clear_overridden, int min_qod = report_counts_build_iterator_min_qod (&cache_iterator); user_t user = report_counts_build_iterator_user (&cache_iterator); + /* Rebuild only what is being invalidated. The iterator yields a row + * for both values of override, but an override cannot change the + * counts that ignore overrides, so when only the overridden counts are + * cleared the other row is still correct and report_counts_id would do + * nothing but read the cache back and throw it away. + * + * Callers that clear neither ask for a plain warm of the whole cache, + * and those still get every row. */ + if ((clear_original || clear_overridden) + && !((clear_original && override == 0) + || (clear_overridden && override))) + continue; + current_credentials.uuid = sql_string ("SELECT uuid FROM users WHERE id = %llu", user); @@ -11611,13 +11653,19 @@ new_severity_clause (int apply_overrides, int dynamic_severity) * @param[in] dynamic_severity Whether to use dynamic severity. * @param[in] filter Filter string. * @param[in] given_new_severity_sql SQL for new severity, or NULL. + * @param[in] bind_report Whether to write the report as $1 instead of + * formatting its rowid into the text, so that + * the statement is the same for every report. + * Only for callers that pass the report to + * init_get_iterator2_with as a bound parameter. * * @return Newly allocated extra_where string. */ static gchar* results_extra_where (int trash, report_t report, const gchar* host, int apply_overrides, int dynamic_severity, - const gchar *filter, const gchar *given_new_severity_sql) + const gchar *filter, const gchar *given_new_severity_sql, + int bind_report) { gchar *extra_where; int min_qod; @@ -11644,8 +11692,11 @@ results_extra_where (int trash, report_t report, const gchar* host, // Build filter clauses - report_clause = report ? g_strdup_printf (" AND (report = %llu) ", report) - : NULL; + report_clause = report + ? (bind_report + ? g_strdup (" AND (report = $1) ") + : g_strdup_printf (" AND (report = %llu) ", report)) + : NULL; if (host) { @@ -11865,13 +11916,15 @@ init_result_get_iterator_severity (iterator_t* iterator, const get_data_t *get, extra_where = results_extra_where (get->trash, report, host, apply_overrides, dynamic_severity, filter ? filter : get->filter, - "lateral_severity"); + "lateral_severity", + 1); extra_where_single = results_extra_where (get->trash, report, host, apply_overrides, dynamic_severity, "min_qod=0", - "lateral_severity"); + "lateral_severity", + 1); free (filter); @@ -11889,6 +11942,18 @@ init_result_get_iterator_severity (iterator_t* iterator, const get_data_t *get, &overrides_with); free (user_id); + /* The report goes in as $1, not as its rowid. This clause used to be + * the most expensive statement in gvmd: rebuilding the count cache + * after an override changes runs it once per affected report, and with + * the rowid in the text every report was a statement PostgreSQL had + * never seen, so it planned the whole thing - the permissions CTE + * included - from scratch every time. Measured over 1064 reports it + * cost 964 ms of the 2100 ms the database spent on the call, nearly + * all of it planning. + * + * With the rowid bound the text is identical for every report, so it + * reaches the prepared statement cache in sql_pg.c and is planned + * once. */ with_clause = g_strdup_printf (" %s," " valid_overrides" @@ -11898,21 +11963,19 @@ init_result_get_iterator_severity (iterator_t* iterator, const get_data_t *get, " WHERE %s" /* Only use if override's NVT is in report. */ " AND EXISTS (SELECT * FROM result_nvt_reports" - " WHERE report = %llu" + " WHERE report = $1" " AND result_nvt" " = overrides.result_nvt)" " AND (task = 0" " OR task = (SELECT reports.task" " FROM reports" - " WHERE reports.id = %llu))" + " WHERE reports.id = $1))" " AND ((end_time = 0) OR (end_time >= m_now ()))" " ORDER BY result DESC, task DESC, port DESC," " severity ASC, creation_time DESC)" " ", overrides_with + strlen ("WITH "), - owned_clause, - report, - report); + owned_clause); g_free (overrides_with); g_free (owned_clause); } @@ -11939,7 +12002,12 @@ init_result_get_iterator_severity (iterator_t* iterator, const get_data_t *get, extra_order, with_clause, 1, - 1); + 1, + /* The report is $1 in extra_where and, when + * overrides apply, in the valid_overrides + * clause. assert (report) above guarantees + * there is a placeholder to bind to. */ + SQL_RESOURCE_PARAM (report)); table_order_if_sort_not_specified = 0; column_array_free (filterable_columns); g_free (with_clause); @@ -12089,13 +12157,13 @@ init_result_get_iterator (iterator_t* iterator, const get_data_t *get, extra_where = results_extra_where (get->trash, report, host, apply_overrides, dynamic_severity, filter ? filter : get->filter, - NULL); + NULL, 0); extra_where_single = results_extra_where (get->trash, report, host, apply_overrides, dynamic_severity, "min_qod=0", - NULL); + NULL, 0); free (filter); @@ -12195,7 +12263,7 @@ result_count (const get_data_t *get, report_t report, const char* host) extra_where = results_extra_where (get->trash, report, host, apply_overrides, dynamic_severity, filter ? filter : get->filter, - NULL); + NULL, 0); ret = count ("result", get, manage_cert_loaded () ? columns : columns_no_cert, @@ -15327,6 +15395,9 @@ tz_revert (gchar *zone, char *tz, char *old_tz_override) sql ("SET SESSION \"gvmd.tz_override\" = %s;", quoted_old_tz_override); g_free (quoted_old_tz_override); + /* tz_override no longer matches what manage_session_init leaves + * behind, so the next call has to write the variables out again. */ + sql_session_uuid_set (NULL); } return 0; } @@ -15408,13 +15479,13 @@ init_delta_iterator (report_t report, iterator_t *results, report_t delta, extra_where = results_extra_where (get->trash, 0, NULL, apply_overrides, dynamic_severity, filter ? filter : get->filter, - NULL); + NULL, 0); extra_where_single = results_extra_where (get->trash, 0, NULL, apply_overrides, dynamic_severity, "min_qod=0", - NULL); + NULL, 0); free (filter); @@ -15522,7 +15593,8 @@ init_delta_iterator (report_t report, iterator_t *results, report_t delta, NULL, extra_with, 0, - 0); + 0, + NULL); g_free (extra_tables); g_free (extra_where); g_free (extra_where_single); @@ -15844,6 +15916,9 @@ print_report_init_zone (print_report_context_t *ctx) quoted_zone = sql_insert (ctx->zone); sql ("SET SESSION \"gvmd.tz_override\" = %s;", quoted_zone); g_free (quoted_zone); + /* tz_override no longer matches what manage_session_init leaves + * behind, so the next call has to write the variables out again. */ + sql_session_uuid_set (NULL); tzset (); } @@ -26796,7 +26871,8 @@ init_vuln_iterator (iterator_t* iterator, const get_data_t *get) NULL, /* extra_order */ extra_with, 0, /* acl_with_optional */ - 0); /* assume_permitted */ + 0, /* assume_permitted */ + NULL); g_free (extra_with); g_free (extra_tables); @@ -27742,7 +27818,7 @@ type_extra_where (const char *type, int trash, const char *filter, apply_overrides, setting_dynamic_severity_int (), filter, - NULL); + NULL, 0); } else if (strcasecmp (type, "VULN") == 0) { diff --git a/src/manage_sql.h b/src/manage_sql.h index 86f87128b..d44db22ca 100644 --- a/src/manage_sql.h +++ b/src/manage_sql.h @@ -385,7 +385,7 @@ init_get_iterator2_with (iterator_t *, const char *, const get_data_t *, column_t *, column_t *, column_t *, column_t *, const char **, int, const char *, const char *, const char *, int, int, const char *, const char *, - int, int); + int, int, const sql_param_t *); int openvasd_get_details_from_iterator (iterator_t *, char **, GSList **); diff --git a/src/manage_sql_assets.c b/src/manage_sql_assets.c index 91b653f5d..6e070c400 100644 --- a/src/manage_sql_assets.c +++ b/src/manage_sql_assets.c @@ -3150,7 +3150,8 @@ init_asset_os_iterator (iterator_t *iterator, const get_data_t *get) NULL, NULL, 0, - 0); + 0, + NULL); g_free (extra_tables); @@ -3401,7 +3402,8 @@ init_resource_names_os_iterator (iterator_t *iterator, get_data_t *get) NULL, NULL, 0, - 0); + 0, + NULL); 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.h b/src/sql.h index 6e47336d5..eb62cd487 100644 --- a/src/sql.h +++ b/src/sql.h @@ -219,6 +219,14 @@ sql_cancel (); int sql_table_exists (const gchar *, const gchar *); +/* Session variables. */ + +const char * +sql_session_uuid (); + +void +sql_session_uuid_set (const char *); + /* Transactions. */ void diff --git a/src/sql_pg.c b/src/sql_pg.c index 6b248b93c..8141b6912 100644 --- a/src/sql_pg.c +++ b/src/sql_pg.c @@ -64,6 +64,95 @@ 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; +} + +/** + * @brief User UUID currently set in the database session, NULL if unknown. + * + * manage_session_init is called far more often than the session actually + * changes. Creating an override walks every report the overridden NVT + * appears in, and each report switches to the owning user, back to the + * caller, and into the ACL check for every user that can see it. Measured + * over 1064 reports that was 25 of the 68 statements per report, and all but + * a handful set the session to the user it was already on. + * + * Caching the UUID here rather than in the manage layer keeps it with the + * connection whose state it describes, so it can be dropped in the same + * places the prepared statements are. SET SESSION is transactional, so a + * rollback undoes it and the cache has to be dropped there too. + */ +static gchar *session_uuid = NULL; + +/** + * @brief Get the user UUID currently set in the database session. + * + * @return The UUID, or NULL if it is not known. + */ +const char * +sql_session_uuid () +{ + return session_uuid; +} + +/** + * @brief Record the user UUID now set in the database session. + * + * Pass NULL to forget the session, which makes the next manage_session_init + * write the variables out again. + * + * @param[in] uuid The UUID, or NULL to forget. + */ +void +sql_session_uuid_set (const char *uuid) +{ + gchar *new_uuid = uuid ? g_strdup (uuid) : NULL; + + g_free (session_uuid); + session_uuid = new_uuid; +} + /* Helpers. */ @@ -259,6 +348,14 @@ sql_open (const db_conn_info_t *database) PostgresPollingStatusType poll_status; int socket; + /* Both caches describe state that lives in a database session, and this is + * about to be a different one. The close paths clear them, but clearing + * here as well means a reconnect that skips the close cannot leave a + * prepared statement name or a user UUID behind that the new session has + * never heard of. */ + sql_prepared_statements_clear (); + sql_session_uuid_set (NULL); + conn_info = g_strdup_printf ( "dbname='%s'" " host='%s'" @@ -378,6 +475,8 @@ sql_open (const db_conn_info_t *database) fail: PQfinish (conn); conn = NULL; + sql_prepared_statements_clear (); + sql_session_uuid_set (NULL); semaphore_op (SEMAPHORE_DB_CONNECTIONS, +1, 0); return -1; } @@ -390,6 +489,8 @@ sql_close () { PQfinish (conn); conn = NULL; + sql_prepared_statements_clear (); + sql_session_uuid_set (NULL); semaphore_op (SEMAPHORE_DB_CONNECTIONS, +1, 0); } @@ -400,6 +501,8 @@ void sql_close_fork () { conn = NULL; + sql_prepared_statements_clear (); + sql_session_uuid_set (NULL); } /** @@ -526,6 +629,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 +700,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 @@ -653,6 +823,9 @@ void sql_rollback () { sql ("ROLLBACK;"); + /* SET SESSION is transactional, so the rollback has just undone whatever + * the transaction set the session variables to. */ + sql_session_uuid_set (NULL); } /**