add function --shadow-builtin flag

It's currently too easy for someone to bork their shell by doing something
like `function test; return 0; end`. That's obviously a silly, contrived,
example but the point is that novice users who learn about functions are
prone to do something like that without realizing it will bork the shell. Even
expert users who know about the `test` builtin might forget that, say, `pwd`
is a builtin.

This change adds a `--shadow-builtin` flag that must be specified to
indicate you know what you're doing.

Fixes #3000

doc_src/function.txt

@@ -29,6 +29,8 @@ The following options are available:
 
 - `-s` or `--on-signal SIGSPEC` tells fish to run this function when the signal SIGSPEC is delivered. SIGSPEC can be a signal number, or the signal name, such as SIGHUP (or just HUP).
 
+- `-B` or `--shadow-builtin` must be specified if the function name is the same as a builtin. Specifying this flag indicates your acknowledgement that you are wrapping or replacing the builtin command. This is a safety feature to make it harder for people to inadvertently break the shell by doing things like `function test; return 0; end`. If the function name is not currently a builtin using this flag will produce an error. If you want to write a function that provides a builtin to an older version of fish you need to add something like `builtin --names | grep -q '^cmd$'; and return` to the top of the function script (where `cmd` is the name of the builtin/function). That will keep your script from replacing the builtin with your function on the newer fish version while allowing your function to provide similar functionality on older versions of fish.
+
 - `-S` or `--no-scope-shadowing` allows the function to access the variables of calling functions. Normally, any variables inside the function that have the same name as variables from the calling function are "shadowed", and their contents is independent of the calling function.
 
 - `-V` or `--inherit-variable NAME` snapshots the value of the variable `NAME` and defines a local variable with that same name and value when the function is executed.

share/functions/__fish_config_interactive.fish

@@ -4,9 +4,7 @@
 # This function is called by the __fish_on_interactive function, which is defined in config.fish.
 #
 function __fish_config_interactive -d "Initializations that should be performed when entering interactive mode"
-
+    # Make sure this function is only run once.
-
-    # Make sure this function is only run once
     if set -q __fish_config_interactive_done
         return
     end
@@ -27,7 +25,6 @@ function __fish_config_interactive -d "Initializations that should be performed
     #
     # If we are starting up for the first time, set various defaults
     #
-
     if not set -q __fish_init_1_50_0
         if not set -q fish_greeting
             set -l line1 (printf (_ 'Welcome to fish, the friendly interactive shell') )
@@ -97,25 +94,22 @@ function __fish_config_interactive -d "Initializations that should be performed
         #
         # Directory history colors
         #
-
         set -q fish_color_history_current
         or set -U fish_color_history_current cyan
     end
 
     #
-    # Generate man page completions if not present
+    # Generate man page completions if not present.
     #
-
     if not test -d $userdatadir/fish/generated_completions
         #fish_update_completions is a function, so it can not be directly run in background.
         eval "$__fish_bin_dir/fish -c 'fish_update_completions > /dev/null ^/dev/null' &"
     end
 
     #
-    # Print a greeting
+    # Print a greeting.
-    # fish_greeting can be a function (preferred) or a variable
+    # fish_greeting can be a function (preferred) or a variable.
     #
-
     if functions -q fish_greeting
         fish_greeting
     else
@@ -130,7 +124,6 @@ function __fish_config_interactive -d "Initializations that should be performed
     # fish_color_cwd changes value. Like all event handlers, it can't be
     # autoloaded.
     #
-
     function __fish_repaint --on-variable fish_color_cwd --description "Event handler, repaints the prompt when fish_color_cwd changes"
         if status --is-interactive
             set -e __fish_prompt_cwd
@@ -149,7 +142,6 @@ function __fish_config_interactive -d "Initializations that should be performed
     # Completions for SysV startup scripts. These aren't bound to any
     # specific command, so they can't be autoloaded.
     #
-
     complete -x -p "/etc/init.d/*" -a start --description 'Start service'
     complete -x -p "/etc/init.d/*" -a stop --description 'Stop service'
     complete -x -p "/etc/init.d/*" -a status --description 'Print service status'
@@ -293,7 +285,7 @@ function __fish_config_interactive -d "Initializations that should be performed
 
         # Don't allow setting color other than what linux offers (see #2001)
         functions -e set_color
-        function set_color
+        function set_color --shadow-builtin
             set -l term_colors black red green yellow blue magenta cyan white normal
             for a in $argv
                 if not contains -- $a $term_colors

share/functions/cd.fish

@@ -1,7 +1,7 @@
 #
 # Wrap the builtin cd command to maintain directory history.
 #
-function cd --description "Change directory"
+function cd --shadow-builtin --description "Change directory"
     set -l MAX_DIR_HIST 25
 
     if test (count $argv) -gt 1

share/functions/history.fish

@@ -1,16 +1,12 @@
 #
-#Deletes an item from history
+# Wrap the builtin history command to provide additional functionality.
 #
-function history --description "Deletes an item from history"
+function history --shadow-builtin --description "Deletes an item from history"
-
     set -l argc (count $argv)
     set -l prefix_args ""
     set -l contains_args ""
-
     set -l cmd print
-
     set -l search_mode none
-
     set -l pager less
     if set -q PAGER
         set pager $PAGER
@@ -46,7 +42,7 @@ function history --description "Deletes an item from history"
             end
         end
     else
-        #Execute history builtin without any argument
+        # Execute history builtin without any argument.
         if status --is-interactive
             builtin history | eval $pager
         else
@@ -57,9 +53,8 @@ function history --description "Deletes an item from history"
 
     switch $cmd
         case print
-            # Print matching items
+            # Print matching items. Note this may end up passing --search twice to the builtin,
-            # Note this may end up passing --search twice to the builtin,
+            # but that's harmless.
-            # but that's harmless
             builtin history --search $argv
 
         case delete
@@ -72,8 +67,7 @@ function history --description "Deletes an item from history"
                     set found_items (builtin history --search --contains $contains_args)
                 case none
                     builtin history $argv
-
+                    # Save changes after deleting item.
-                    #Save changes after deleting item
                     builtin history --save
                     return 0
             end
@@ -98,7 +92,8 @@ function history --description "Deletes an item from history"
                         continue
                     end
 
-                    #Following two validations could be embedded with "and" but I find the syntax kind of weird.
+                    # Following two validations could be embedded with "and" but I find the syntax
+                    # kind of weird.
                     if not string match -qr '^[0-9]+$' $i
                         printf "Invalid input: %s\n" $i
                         continue
@@ -124,18 +119,18 @@ function history --description "Deletes an item from history"
 
                     end
                 end
-                #Save changes after deleting item(s)
+                # Save changes after deleting item(s).
                 builtin history --save
             end
         case save
-            #Save changes to history file
+            # Save changes to history file.
             builtin history $argv
         case merge
             builtin history --merge
         case help
             builtin history --help
         case clear
-            # Erase the entire history
+            # Erase the entire history.
             echo "Are you sure you want to clear history ? (y/n)"
             read ch
             if test $ch = "y"

src/builtin.cpp

@@ -889,6 +889,7 @@ static int builtin_generic(parser_t &parser, io_streams_t &streams, wchar_t **ar
             }
         }
     }
+
     return STATUS_BUILTIN_ERROR;
 }
 
@@ -919,7 +920,11 @@ static wcstring functions_def(const wcstring &name) {
         out.append(esc_desc);
     }
 
-    if (!function_get_shadows(name)) {
+    if (function_get_shadow_builtin(name)) {
+        out.append(L" --shadow-builtin");
+    }
+
+    if (!function_get_shadow_scope(name)) {
         out.append(L" --no-scope-shadowing");
     }
 
@@ -1460,7 +1465,7 @@ static int builtin_pwd(parser_t &parser, io_streams_t &streams, wchar_t **argv)
 }
 
 /// Adds a function to the function set. It calls into function.cpp to perform any heavy lifting.
-int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
+int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
                      const wcstring &contents, int definition_line_offset, wcstring *out_err) {
     wgetopter_t w;
     assert(out_err != NULL);
@@ -1484,7 +1489,8 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
     wcstring_list_t named_arguments;
     wcstring_list_t inherit_vars;
 
-    bool shadows = true;
+    bool shadow_builtin = false;
+    bool shadow_scope = true;
 
     wcstring_list_t wrap_targets;
 
@@ -1504,17 +1510,17 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
                                            {L"wraps", required_argument, 0, 'w'},
                                            {L"help", no_argument, 0, 'h'},
                                            {L"argument-names", no_argument, 0, 'a'},
+                                           {L"shadow-builtin", no_argument, 0, 'B'},
                                            {L"no-scope-shadowing", no_argument, 0, 'S'},
                                            {L"inherit-variable", required_argument, 0, 'V'},
                                            {0, 0, 0, 0}};
 
-    while (1 && (!res)) {
+    while (1 && !res) {
         int opt_index = 0;
 
         // The leading - here specifies RETURN_IN_ORDER.
-        int opt = w.wgetopt_long(argc, argv, L"-d:s:j:p:v:e:w:haSV:", long_options, &opt_index);
+        int opt = w.wgetopt_long(argc, argv, L"-d:s:j:p:v:e:w:haBSV:", long_options, &opt_index);
         if (opt == -1) break;
-
         switch (opt) {
             case 0: {
                 if (long_options[opt_index].flag != 0) break;
@@ -1536,7 +1542,6 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
                 events.push_back(event_t::signal_event(sig));
                 break;
             }
-
             case 'v': {
                 if (wcsvarname(w.woptarg)) {
                     append_format(*out_err, _(L"%ls: Invalid variable name '%ls'"), argv[0],
@@ -1586,7 +1591,6 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
                         e.type = EVENT_JOB_ID;
                         e.param1.job_id = job_id;
                     }
-
                 } else {
                     errno = 0;
                     pid = fish_wcstoi(w.woptarg, &end, 10);
@@ -1611,8 +1615,12 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
                 name_is_first_positional = !positionals.empty();
                 break;
             }
+            case 'B': {
+                shadow_builtin = true;
+                break;
+            }
             case 'S': {
-                shadows = 0;
+                shadow_scope = false;
                 break;
             }
             case 'w': {
@@ -1699,6 +1707,30 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
             }
         }
 
+        if (!res) {
+            bool function_name_shadows_builtin = false;
+            wcstring_list_t builtin_names = builtin_get_names();
+            for (size_t i = 0; i < builtin_names.size(); i++) {
+                const wchar_t *el = builtin_names.at(i).c_str();
+                if (el == function_name) {
+                    function_name_shadows_builtin = true;
+                    break;
+                }
+            }
+            if (function_name_shadows_builtin && !shadow_builtin) {
+                append_format(
+                    *out_err,
+                    _(L"%ls: function name shadows a builtin so you must use '--shadow-builtin'"),
+                    argv[0]);
+                res = STATUS_BUILTIN_ERROR;
+            } else if (!function_name_shadows_builtin && shadow_builtin) {
+                append_format(*out_err, _(L"%ls: function name does not shadow a builtin so you "
+                                          L"must not use '--shadow-builtin'"),
+                              argv[0]);
+                res = STATUS_BUILTIN_ERROR;
+            }
+        }
+
         if (!res) {
             // Here we actually define the function!
             function_data_t d;
@@ -1706,7 +1738,8 @@ int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list
             d.name = function_name;
             if (desc) d.description = desc;
             d.events.swap(events);
-            d.shadows = shadows;
+            d.shadow_builtin = shadow_builtin;
+            d.shadow_scope = shadow_scope;
             d.named_arguments.swap(named_arguments);
             d.inherit_vars.swap(inherit_vars);
 

src/builtin.h

@@ -93,9 +93,9 @@ class builtin_commandline_scoped_transient_t {
 // Run the __fish_print_help function to obtain the help information for the specified command.
 wcstring builtin_help_get(parser_t &parser, const wchar_t *cmd);
 
-// Defines a function, like builtin_function. Returns 0 on success. args should NOT contain
+// Defines a function. Returns 0 on success. args should NOT contain 'function' as the first
-// 'function' as the first argument.
+// argument as the parser treats it as a keyword.
-int define_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
+int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
                      const wcstring &contents, int definition_line_offset, wcstring *out_err);
 
 // Print help for the specified builtin. If \c b is sb_err, also print the line information.

src/exec.cpp

@@ -636,7 +636,7 @@ void exec_job(parser_t &parser, job_t *j) {
                 wcstring def;
                 bool function_exists = function_get_definition(func_name, &def);
 
-                bool shadows = function_get_shadows(func_name);
+                bool shadow_scope = function_get_shadow_scope(func_name);
                 const std::map<wcstring, env_var_t> inherit_vars =
                     function_get_inherit_vars(func_name);
 
@@ -646,7 +646,7 @@ void exec_job(parser_t &parser, job_t *j) {
                     debug(0, _(L"Unknown function '%ls'"), p->argv0());
                     break;
                 }
-                function_block_t *newv = new function_block_t(p, func_name, shadows);
+                function_block_t *newv = new function_block_t(p, func_name, shadow_scope);
                 parser.push_block(newv);
 
                 // Setting variables might trigger an event handler, hence we need to unblock

src/function.cpp

@@ -141,7 +141,8 @@ function_info_t::function_info_t(const function_data_t &data, const wchar_t *fil
       named_arguments(data.named_arguments),
       inherit_vars(snapshot_vars(data.inherit_vars)),
       is_autoload(autoload),
-      shadows(data.shadows) {}
+      shadow_builtin(data.shadow_builtin),
+      shadow_scope(data.shadow_scope) {}
 
 function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename,
                                  int def_offset, bool autoload)
@@ -152,7 +153,8 @@ function_info_t::function_info_t(const function_info_t &data, const wchar_t *fil
       named_arguments(data.named_arguments),
       inherit_vars(data.inherit_vars),
       is_autoload(autoload),
-      shadows(data.shadows) {}
+      shadow_builtin(data.shadow_builtin),
+      shadow_scope(data.shadow_scope) {}
 
 void function_add(const function_data_t &data, const parser_t &parser, int definition_line_offset) {
     ASSERT_IS_MAIN_THREAD();
@@ -255,10 +257,16 @@ std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name) {
     return func ? func->inherit_vars : std::map<wcstring, env_var_t>();
 }
 
-int function_get_shadows(const wcstring &name) {
+int function_get_shadow_builtin(const wcstring &name) {
     scoped_lock lock(functions_lock);
     const function_info_t *func = function_get(name);
-    return func ? func->shadows : false;
+    return func ? func->shadow_builtin : false;
+}
+
+int function_get_shadow_scope(const wcstring &name) {
+    scoped_lock lock(functions_lock);
+    const function_info_t *func = function_get(name);
+    return func ? func->shadow_scope : false;
 }
 
 bool function_get_desc(const wcstring &name, wcstring *out_desc) {

src/function.h

@@ -31,12 +31,34 @@ struct function_data_t {
     /// List of all variables that are inherited from the function definition scope. The variable
     /// values are snapshotted when function_add() is called.
     wcstring_list_t inherit_vars;
-    /// Set to non-zero if invoking this function shadows the variables of the underlying function.
+    /// Set to true if invoking this function shadows the variables of the underlying function.
-    int shadows;
+    bool shadow_scope;
+    /// Set to true if this function shadows a builtin.
+    bool shadow_builtin;
 };
 
 class function_info_t {
    public:
+    /// Function definition.
+    const wcstring definition;
+    /// Function description. Only the description may be changed after the function is created.
+    wcstring description;
+    /// File where this function was defined (intern'd string).
+    const wchar_t *const definition_file;
+    /// Line where definition started.
+    const int definition_offset;
+    /// List of all named arguments for this function.
+    const wcstring_list_t named_arguments;
+    /// Mapping of all variables that were inherited from the function definition scope to their
+    /// values.
+    const std::map<wcstring, env_var_t> inherit_vars;
+    /// Flag for specifying that this function was automatically loaded.
+    const bool is_autoload;
+    /// Set to true if this function shadows a builtin.
+    const bool shadow_builtin;
+    /// Set to true if invoking this function shadows the variables of the underlying function.
+    const bool shadow_scope;
+
     /// Constructs relevant information from the function_data.
     function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset,
                     bool autoload);
@@ -44,31 +66,6 @@ class function_info_t {
     /// Used by function_copy.
     function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset,
                     bool autoload);
-
-    /// Function definition.
-    const wcstring definition;
-
-    /// Function description. Only the description may be changed after the function is created.
-    wcstring description;
-
-    /// File where this function was defined (intern'd string).
-    const wchar_t *const definition_file;
-
-    /// Line where definition started.
-    const int definition_offset;
-
-    /// List of all named arguments for this function.
-    const wcstring_list_t named_arguments;
-
-    /// Mapping of all variables that were inherited from the function definition scope to their
-    /// values.
-    const std::map<wcstring, env_var_t> inherit_vars;
-
-    /// Flag for specifying that this function was automatically loaded.
-    const bool is_autoload;
-
-    /// Set to true if invoking this function shadows the variables of the underlying function.
-    const bool shadows;
 };
 
 /// Initialize function data.
@@ -133,8 +130,11 @@ std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name);
 /// is successful.
 bool function_copy(const wcstring &name, const wcstring &new_name);
 
+/// Returns whether this function shadows a builtin of the same name.
+int function_get_shadow_builtin(const wcstring &name);
+
 /// Returns whether this function shadows variables of the underlying function.
-int function_get_shadows(const wcstring &name);
+int function_get_shadow_scope(const wcstring &name);
 
 /// Prepares the environment for executing a function.
 void function_prepare_environment(const wcstring &name, const wchar_t *const *argv,

src/parse_execution.cpp

@@ -392,7 +392,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement(
         int definition_line_offset = this->line_offset_of_character_at_offset(contents_start);
         wcstring error_str;
         io_streams_t streams;
-        int err = define_function(*parser, streams, argument_list, contents_str,
+        int err = builtin_function(*parser, streams, argument_list, contents_str,
                                    definition_line_offset, &error_str);
         proc_set_last_status(err);
 

tests/function.err

@@ -0,0 +1,8 @@
+function: function name shadows a builtin so you must use '--shadow-builtin'
+fish: function pwd; end
+      ^
+yes, it failed as expected
+function: function name does not shadow a builtin so you must not use '--shadow-builtin'
+fish: function not_builtin --shadow-builtin; end
+      ^
+yes, it failed as expected

tests/function.in

@@ -44,3 +44,16 @@ for i in (seq 4)
         echo "Function name$i not found, but should have been"
     end
 end
+
+# Test that we can't define a function that shadows a builtin by accident.
+function pwd; end
+or echo 'yes, it failed as expected' >&2
+
+# Test that we can define a function that shadows a builtin if we use the
+# right flag.
+function pwd --shadow-builtin; end
+and echo '"function pwd --shadow-builtin" worked'
+
+# Using --shadow-builtin for a non-builtin function name also fails.
+function not_builtin --shadow-builtin; end
+or echo 'yes, it failed as expected' >&2

tests/function.out

@@ -22,3 +22,4 @@ Function name1 found
 Function name2 found
 Function name3 found
 Function name4 found
+"function pwd --shadow-builtin" worked