Improve nasl linter

CHANGELOG.md

@@ -7,6 +7,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## [21.04.1] (unreleased)
 
 ### Added
+- Improve nasl linter to catch more cases of undeclared variables. [#728][(https://github.com/greenbone/openvas-scanner/pull/728)
+
 ### Changed
 - Update default log config [#711](https://github.com/greenbone/openvas-scanner/pull/711)
 

nasl/exec.c

@@ -1618,7 +1618,8 @@ nasl_lint (lex_ctxt *, tree_cell *);
  *                          parse-only, always-signed, command-line, lint)
  *
  * @return 0 if the script was executed successfully, negative values if an
- * error occurred.
+ * error occurred. Return number of errors if mode is NASL_LINT and no none
+ * linting errors occured.
  */
 int
 exec_nasl_script (struct script_infos *script_infos, int mode)
@@ -1698,8 +1699,18 @@ exec_nasl_script (struct script_infos *script_infos, int mode)
   process_id = getpid ();
   if (mode & NASL_LINT)
     {
-      if (nasl_lint (lexic, ctx.tree) == NULL)
+      /* ret is set to the number of errors the linter finds.
+      ret will be overwritten with -1 if any erros occur in the steps
+      after linting so we do not break other behaviour dependent on a
+      negative return value when doing more than just linting. */
+      tree_cell *ret = nasl_lint (lexic, ctx.tree);
+      if (ret == NULL)
         err--;
+      else if (ret != FAKE_CELL && ret->x.i_val > 0)
+        {
+          err = ret->x.i_val;
+          g_free (ret);
+        }
     }
   else if (!(mode & NASL_EXEC_PARSE_ONLY))
     {

nasl/lint.c

@@ -47,6 +47,24 @@ typedef struct st_func_info
 
 char *nasl_name;
 
+int errors_cnt;
+void
+init_errors_cnt ()
+{
+  errors_cnt = 0;
+}
+void
+inc_errors_cnt ()
+{
+  errors_cnt++;
+  return;
+}
+int
+get_errors_cnt ()
+{
+  return errors_cnt;
+}
+
 /**
  * @brief Free a func_info structure.
  *
@@ -141,10 +159,10 @@ reverse_search (GSList **def_func_tree, GSList *finfo)
 gint
 list_cmp (gconstpointer lelem, gconstpointer data)
 {
-  if (data)
-    return (g_strcmp0 (lelem, data));
+  if (!lelem || !data)
+    return -1;
 
-  return -1;
+  return (strcasecmp (lelem, data));
 }
 
 /**
@@ -179,6 +197,7 @@ print_uncall_files (gpointer filename, gpointer lexic)
     {
       nasl_perror (lexic, "The included file '%s' is never used.",
                    (char *) filename);
+      inc_errors_cnt ();
       lexic = NULL;
     }
 }
@@ -416,9 +435,13 @@ nasl_lint_defvar (lex_ctxt *lexic, tree_cell *st, GHashTable **include_files,
        || st->type == NODE_PLUS_EQ)
       && defined_var_mode == 0)
     defined_var_mode = 1;
-  else if ((st->type == NODE_FUN_DEF || st->type == NODE_LOCAL)
+  else if ((st->type == NODE_FUN_DEF || st->type == NODE_LOCAL
+            || st->type == NODE_FUN_CALL)
            && defined_fn_mode == 0)
-    defined_fn_mode = 1;
+    {
+      defined_fn_mode = 1;
+      defined_var_mode = 0;
+    }
 
   else if (st->type == NODE_GLOBAL)
     def_glob_var = 1;
@@ -443,18 +466,35 @@ nasl_lint_defvar (lex_ctxt *lexic, tree_cell *st, GHashTable **include_files,
   else if (st->type == NODE_DECL && st->x.str_val != NULL)
     {
       if (defined_fn_mode == 1)
-        local_var_list = g_slist_prepend (local_var_list, st->x.str_val);
+        {
+          local_var_list = g_slist_prepend (local_var_list, st->x.str_val);
+        }
       if (def_glob_var == 1)
-        *defined_var = g_slist_prepend (*defined_var, st->x.str_val);
+        {
+          *defined_var = g_slist_prepend (*defined_var, st->x.str_val);
+        }
     }
   /* Special case foreach. */
   else if (st->type == NODE_FOREACH)
     {
-      if (st->x.str_val != NULL)
-        *defined_var = g_slist_prepend (*defined_var, st->x.str_val);
+      // Hacky way of checking if we are in a function definition by checking
+      // if local_var_list is non empty. Otherwise all variables declared in a
+      // foreach call are considered file scope which leads to false negatives.
+      if (st->x.str_val != NULL && local_var_list != NULL)
+        {
+          local_var_list = g_slist_prepend (local_var_list, st->x.str_val);
+        }
+      else if (st->x.str_val != NULL)
+        {
+          *defined_var = g_slist_prepend (*defined_var, st->x.str_val);
+        }
     }
+
   // The variable is used. It checks if the variable was defined
-  else if (st->type == NODE_VAR && defined_var_mode == 0)
+  // Also check for NODE_ARRAY_EL to catch use of undeclared array.
+  // E.g "if(foo[0]) {}" and foo was not declared previously.
+  else if ((st->type == NODE_VAR || st->type == NODE_ARRAY_EL)
+           && defined_var_mode == 0)
     {
       if (!g_slist_find_custom (*defined_var, st->x.str_val,
                                 (GCompareFunc) list_cmp)
@@ -464,7 +504,7 @@ nasl_lint_defvar (lex_ctxt *lexic, tree_cell *st, GHashTable **include_files,
           lexic->line_nb = st->line_nb;
           nasl_perror (lexic, "The variable %s was not declared",
                        st->x.str_val);
-          return NULL;
+          inc_errors_cnt ();
         }
     }
 
@@ -611,7 +651,8 @@ find_description_block (lex_ctxt *lexic, tree_cell *st)
  * @param[in] lexic nasl context.
  * @param[in] st structure tree of a nasl script.
  *
- * @return FAKE_CELL if no error was found, NULL otherwise.
+ * @return FAKE_CELL if no error was found, otherwise NULL or tree_cell which
+ *  has number of errors as x.i_val.
  */
 tree_cell *
 nasl_lint (lex_ctxt *lexic, tree_cell *st)
@@ -626,6 +667,7 @@ nasl_lint (lex_ctxt *lexic, tree_cell *st)
   GSList *def_func_tree = NULL;
   gchar *err_fname = NULL;
   tree_cell *desc_block = FAKE_CELL;
+  init_errors_cnt ();
 
   nasl_name = g_strdup (nasl_get_filename (st->x.str_val));
   include_files =
@@ -644,7 +686,10 @@ nasl_lint (lex_ctxt *lexic, tree_cell *st)
   if (desc_block != NULL && desc_block != FAKE_CELL)
     /* FAKE_CELL if success, NULL otherwise which counts as error */
     if ((ret = check_description_block (lexic_aux, desc_block)) == NULL)
-      goto fail;
+      {
+        inc_errors_cnt ();
+        goto fail;
+      }
 
   /* Make a list of all called functions */
   make_call_func_list (lexic_aux, st, &called_funcs);
@@ -654,12 +699,18 @@ nasl_lint (lex_ctxt *lexic, tree_cell *st)
                             &func_fnames_tab, err_fname, &called_funcs,
                             &def_func_tree))
       == NULL)
-    goto fail;
+    {
+      inc_errors_cnt ();
+      goto fail;
+    }
   /* Check if a called function was defined. */
   if ((ret = nasl_lint_call (lexic_aux, st, &include_files, &func_fnames_tab,
                              err_fname, &called_funcs, &def_func_tree))
       == NULL)
-    goto fail;
+    {
+      inc_errors_cnt ();
+      goto fail;
+    }
 
   /* Check if the included files are used or not. */
   g_hash_table_foreach (include_files, (GHFunc) check_called_files,
@@ -678,7 +729,10 @@ nasl_lint (lex_ctxt *lexic, tree_cell *st)
          nasl_lint_def (lexic, st, lint_mode, &include_files, &func_fnames_tab,
                         err_fname, &called_funcs, &def_func_tree))
       == NULL)
-    goto fail;
+    {
+      inc_errors_cnt ();
+      goto fail;
+    }
 
   /* Check if a variable was declared. */
   GSList *defined_var = NULL;
@@ -702,5 +756,11 @@ nasl_lint (lex_ctxt *lexic, tree_cell *st)
   unusedfiles = NULL;
   free_lex_ctxt (lexic_aux);
 
+  if (get_errors_cnt () > 0)
+    {
+      ret = alloc_typed_cell (NODE_VAR);
+      ret->x.i_val = get_errors_cnt ();
+    }
+
   return ret;
 }

nasl/nasl-lint.c

@@ -60,27 +60,32 @@ get_DIS_from_filename (const gchar *filename)
  * @brief Process a file through the linter
  * @param filepath the path of the file to be processed
  * @param mode,script_args The parameters to be given to the linter
- * @return TRUE if the file contains error(s)
+ * @return Number of errors in script
  */
-static gboolean
+static int
 process_file (const gchar *filepath, int mode, struct script_infos *script_args)
 {
+  int ret;
+
   g_debug ("Processing %s", filepath);
   script_args->name = (char *) filepath;
-  if (exec_nasl_script (script_args, mode) < 0)
+  ret = exec_nasl_script (script_args, mode);
+  if (ret != 0)
     {
       g_print ("Error while processing %s.\n", filepath);
-      return TRUE;
+      if (ret == -1)
+        return 1;
+      return ret;
     }
-  return FALSE;
+  return 0;
 }
 
 /**
  * @brief Process each files in the list_file through the linter
  * @param list_file the path to a text file containing path to the files to
  *        process, one per line
  * @param mode,script_args Parameters for the linter
- * @return The amount of scripts that contain errors
+ * @return The amount of errors found in the given scripts
  */
 static int
 process_file_list (const gchar *list_file, int mode,
@@ -105,8 +110,7 @@ process_file_list (const gchar *list_file, int mode,
       if (line == NULL)
         break;
 
-      if (process_file (line, mode, script_args))
-        err++;
+      err += process_file (line, mode, script_args);
 
       g_free (line);
     }
@@ -119,7 +123,7 @@ process_file_list (const gchar *list_file, int mode,
  * @brief Process each given files through the linter
  * @param files The path to the files to be processed
  * @param mode,script_args Parameters to be given to the linter
- * @return The amount of script that contains errors
+ * @return The amount of errors found in the given scripts
  */
 static int
 process_files (const gchar **files, int mode, struct script_infos *script_args)
@@ -128,8 +132,7 @@ process_files (const gchar **files, int mode, struct script_infos *script_args)
   int err = 0;
   while (files[n])
     {
-      if (process_file (files[n], mode, script_args))
-        err++;
+      err += process_file (files[n], mode, script_args);
       n++;
     }
   return err;