From b1f681ee11668c38b54d5dfe16b72b406729395b Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Sat, 25 Apr 2020 08:43:26 +0200 Subject: [PATCH] new warning - almost all plpgsql auto variables (without new and old) should not be modified by user --- README.md | 2 +- src/assign.c | 21 ++++++++++++++++++++ src/check_function.c | 47 +++++++++++++++++++++++++++----------------- src/plpgsql_check.h | 4 +++- src/report.c | 11 ++++++++--- src/stmtwalk.c | 4 ++++ src/typdesc.c | 2 ++ 7 files changed, 68 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index b90d943..ceb1259 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ You can set level of warnings via function's parameters: * `extra_warnings boolean DEFAULT true` - show warnings like missing `RETURN`, shadowed variables, dead code, never read (unused) function's parameter, - unmodified variables, .. + unmodified variables, modified auto variables, .. * `performance_warnings boolean DEFAULT false` - performance related warnings like declared type with type modificator, casting, implicit casts in where clause (can be diff --git a/src/assign.c b/src/assign.c index bbb770b..6594dfc 100644 --- a/src/assign.c +++ b/src/assign.c @@ -42,7 +42,28 @@ plpgsql_check_record_variable_usage(PLpgSQL_checkstate *cstate, int dno, bool wr if (!write) cstate->used_variables = bms_add_member(cstate->used_variables, dno); else + { cstate->modif_variables = bms_add_member(cstate->modif_variables, dno); + + /* raise extra warning when protected variable is modified */ + if (bms_is_member(dno, cstate->protected_variables)) + { + PLpgSQL_variable *var = (PLpgSQL_variable *) cstate->estate->datums[dno]; + StringInfoData message; + + initStringInfo(&message); + + appendStringInfo(&message, "auto varible \"%s\" should not be modified by user", var->refname); + plpgsql_check_put_error(cstate, + 0, var->lineno, + message.data, + NULL, + NULL, + PLPGSQL_CHECK_WARNING_EXTRA, + 0, NULL, NULL); + pfree(message.data); + } + } } } diff --git a/src/check_function.c b/src/check_function.c index 3f758d9..50b2578 100644 --- a/src/check_function.c +++ b/src/check_function.c @@ -53,7 +53,7 @@ static void function_check(PLpgSQL_function *func, PLpgSQL_checkstate *cstate); static void trigger_check(PLpgSQL_function *func, Node *tdata, PLpgSQL_checkstate *cstate); static void release_exprs(List *exprs); static int load_configuration(HeapTuple procTuple, bool *reload_config); -static void init_datum_dno(PLpgSQL_checkstate *cstate, int dno); +static void init_datum_dno(PLpgSQL_checkstate *cstate, int dno, bool is_auto, bool is_protected); static PLpgSQL_datum * copy_plpgsql_datum(PLpgSQL_checkstate *cstate, PLpgSQL_datum *datum); static void plpgsql_check_setup_estate(PLpgSQL_execstate *estate, PLpgSQL_function *func, ReturnSetInfo *rsi, plpgsql_check_info *cinfo); static void plpgsql_check_setup_cstate(PLpgSQL_checkstate *cstate, plpgsql_check_result_info *result_info, @@ -72,7 +72,7 @@ collect_out_variables(PLpgSQL_function *func, PLpgSQL_checkstate *cstate) int varno = func->out_param_varno; PLpgSQL_variable *var = (PLpgSQL_variable *) func->datums[varno]; - if (var->dtype == PLPGSQL_DTYPE_ROW && is_internal_variable(var)) + if (var->dtype == PLPGSQL_DTYPE_ROW && is_internal_variable(cstate, var)) { /* this function has more OUT parameters */ PLpgSQL_row *row = (PLpgSQL_row*) var; @@ -521,6 +521,8 @@ function_check(PLpgSQL_function *func, PLpgSQL_checkstate *cstate) for (i = 0; i < cstate->estate->ndatums; i++) cstate->estate->datums[i] = copy_plpgsql_datum(cstate, func->datums[i]); + init_datum_dno(cstate, cstate->estate->found_varno, true, true); + /* * check function's parameters to not be reserved keywords */ @@ -553,7 +555,7 @@ function_check(PLpgSQL_function *func, PLpgSQL_checkstate *cstate) */ for (i = 0; i < func->fn_nargs; i++) { - init_datum_dno(cstate, func->fn_argvarnos[i]); + init_datum_dno(cstate, func->fn_argvarnos[i], false, false); } /* @@ -600,6 +602,8 @@ trigger_check(PLpgSQL_function *func, Node *tdata, PLpgSQL_checkstate *cstate) for (i = 0; i < cstate->estate->ndatums; i++) cstate->estate->datums[i] = copy_plpgsql_datum(cstate, func->datums[i]); + init_datum_dno(cstate, cstate->estate->found_varno, true, true); + if (IsA(tdata, TriggerData)) { TriggerData *trigdata = (TriggerData *) tdata; @@ -624,7 +628,7 @@ trigger_check(PLpgSQL_function *func, Node *tdata, PLpgSQL_checkstate *cstate) PLpgSQL_datum *datum = func->datums[i]; if (datum->dtype == PLPGSQL_DTYPE_PROMISE) - init_datum_dno(cstate, datum->dno); + init_datum_dno(cstate, datum->dno, true, datum->dno != func->new_varno && datum->dno != func->old_varno); } rec_new = (PLpgSQL_rec *) (cstate->estate->datums[func->new_varno]); @@ -647,16 +651,16 @@ trigger_check(PLpgSQL_function *func, Node *tdata, PLpgSQL_checkstate *cstate) /* * Assign the special tg_ variables */ - init_datum_dno(cstate, func->tg_op_varno); - init_datum_dno(cstate, func->tg_name_varno); - init_datum_dno(cstate, func->tg_when_varno); - init_datum_dno(cstate, func->tg_level_varno); - init_datum_dno(cstate, func->tg_relid_varno); - init_datum_dno(cstate, func->tg_relname_varno); - init_datum_dno(cstate, func->tg_table_name_varno); - init_datum_dno(cstate, func->tg_table_schema_varno); - init_datum_dno(cstate, func->tg_nargs_varno); - init_datum_dno(cstate, func->tg_argv_varno); + init_datum_dno(cstate, func->tg_op_varno, true, true); + init_datum_dno(cstate, func->tg_name_varno, true, true); + init_datum_dno(cstate, func->tg_when_varno, true, true); + init_datum_dno(cstate, func->tg_level_varno, true, true); + init_datum_dno(cstate, func->tg_relid_varno, true, true); + init_datum_dno(cstate, func->tg_relname_varno, true, true); + init_datum_dno(cstate, func->tg_table_name_varno, true, true); + init_datum_dno(cstate, func->tg_table_schema_varno, true, true); + init_datum_dno(cstate, func->tg_nargs_varno, true, true); + init_datum_dno(cstate, func->tg_argv_varno, true, true); #endif @@ -666,8 +670,8 @@ trigger_check(PLpgSQL_function *func, Node *tdata, PLpgSQL_checkstate *cstate) #if PG_VERSION_NUM < 110000 - init_datum_dno(cstate, func->tg_event_varno); - init_datum_dno(cstate, func->tg_tag_varno); + init_datum_dno(cstate, func->tg_event_varno, true, true); + init_datum_dno(cstate, func->tg_tag_varno, true, true); #endif @@ -1042,6 +1046,8 @@ plpgsql_check_setup_cstate(PLpgSQL_checkstate *cstate, cstate->fake_rtd = fake_rtd; cstate->safe_variables = NULL; + cstate->protected_variables = NULL; + cstate->auto_variables = NULL; cstate->stop_check = false; cstate->allow_mp = false; @@ -1109,7 +1115,7 @@ release_exprs(List *exprs) * */ static void -init_datum_dno(PLpgSQL_checkstate *cstate, int dno) +init_datum_dno(PLpgSQL_checkstate *cstate, int dno, bool is_auto, bool is_protected) { switch (cstate->estate->datums[dno]->dtype) { @@ -1153,7 +1159,7 @@ init_datum_dno(PLpgSQL_checkstate *cstate, int dno) if (row->varnos[fnum] < 0) continue; /* skip dropped column in row struct */ - init_datum_dno(cstate, row->varnos[fnum]); + init_datum_dno(cstate, row->varnos[fnum], is_auto, is_protected); } } break; @@ -1161,6 +1167,11 @@ init_datum_dno(PLpgSQL_checkstate *cstate, int dno) default: elog(ERROR, "unexpected dtype: %d", cstate->estate->datums[dno]->dtype); } + + if (is_protected) + cstate->protected_variables = bms_add_member(cstate->protected_variables, dno); + if (is_auto) + cstate->auto_variables = bms_add_member(cstate->auto_variables, dno); } /* diff --git a/src/plpgsql_check.h b/src/plpgsql_check.h index 7646677..d049e53 100644 --- a/src/plpgsql_check.h +++ b/src/plpgsql_check.h @@ -103,6 +103,8 @@ typedef struct PLpgSQL_checkstate plpgsql_check_info *cinfo; Bitmapset *safe_variables; /* track which variables are safe against sql injection */ Bitmapset *out_variables; /* what variables are used as OUT variables */ + Bitmapset *protected_variables; /* what variables should be assigned internal only */ + Bitmapset *auto_variables; /* variables initialized, used by runtime */ bool stop_check; /* true after error when fatal_errors option is active */ bool allow_mp; /* true, when multiple plans in plancache are allowed */ bool has_mp; /* true, when multiple plan was used */ @@ -229,7 +231,7 @@ extern void plpgsql_check_assignment_to_variable(PLpgSQL_checkstate *cstate, PLp extern char * plpgsql_check_datum_get_refname(PLpgSQL_datum *d); extern void plpgsql_check_report_unused_variables(PLpgSQL_checkstate *cstate); extern void plpgsql_check_report_too_high_volatility(PLpgSQL_checkstate *cstate); -extern bool is_internal_variable(PLpgSQL_variable *var); +extern bool is_internal_variable(PLpgSQL_checkstate *cstate, PLpgSQL_variable *var); /* * functions from stmtwalk.c diff --git a/src/report.c b/src/report.c index 9651666..84bbd28 100644 --- a/src/report.c +++ b/src/report.c @@ -35,8 +35,11 @@ is_internal(char *refname, int lineno) } bool -is_internal_variable(PLpgSQL_variable *var) +is_internal_variable(PLpgSQL_checkstate *cstate, PLpgSQL_variable *var) { + if (bms_is_member(var->dno, cstate->auto_variables)) + return true; + return is_internal(var->refname, var->lineno); } @@ -91,6 +94,9 @@ datum_is_explicit(PLpgSQL_checkstate *cstate, int dno) { PLpgSQL_execstate *estate = cstate->estate; + if (bms_is_member(dno, cstate->auto_variables)) + return false; + switch (estate->datums[dno]->dtype) { case PLPGSQL_DTYPE_VAR: @@ -229,7 +235,6 @@ plpgsql_check_report_unused_variables(PLpgSQL_checkstate *cstate) PLpgSQL_function *func = estate->func; /* check never read variables */ - for (i = 0; i < estate->ndatums; i++) { if (datum_is_explicit(cstate, i) @@ -320,7 +325,7 @@ plpgsql_check_report_unused_variables(PLpgSQL_checkstate *cstate) int varno = func->out_param_varno; PLpgSQL_variable *var = (PLpgSQL_variable *) estate->datums[varno]; - if (var->dtype == PLPGSQL_DTYPE_ROW && is_internal_variable(var)) + if (var->dtype == PLPGSQL_DTYPE_ROW && is_internal_variable(cstate, var)) { /* this function has more OUT parameters */ PLpgSQL_row *row = (PLpgSQL_row*) var; diff --git a/src/stmtwalk.c b/src/stmtwalk.c index cda8b36..d223f6b 100644 --- a/src/stmtwalk.c +++ b/src/stmtwalk.c @@ -623,6 +623,10 @@ plpgsql_check_stmt(PLpgSQL_checkstate *cstate, PLpgSQL_stmt *stmt, int *closing, if (stmt_fori->step) plpgsql_check_assignment(cstate, stmt_fori->step, NULL, NULL, dno); + /* this variable should not be updated */ + cstate->protected_variables = bms_add_member(cstate->protected_variables, dno); + cstate->auto_variables = bms_add_member(cstate->auto_variables, dno); + check_stmts(cstate, stmt_fori->body, &closing_local, &exceptions_local); *closing = possibly_closed(closing_local); } diff --git a/src/typdesc.c b/src/typdesc.c index 8963e74..6f17ff0 100644 --- a/src/typdesc.c +++ b/src/typdesc.c @@ -84,6 +84,8 @@ plpgsql_check_CallExprGetRowTarget(PLpgSQL_checkstate *cstate, PLpgSQL_expr *Cal row = palloc0(sizeof(PLpgSQL_row)); row->dtype = PLPGSQL_DTYPE_ROW; + row->dno = -1; + row->refname = NULL; row->lineno = 0; row->varnos = palloc(sizeof(int) * list_length(funcargs));