diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9ba252fe2..be06eaaed 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -185,6 +185,7 @@ Scripting improvements - ``fish --profile`` now only starts profiling after fish is ready to execute commands (all configuration is completed). There is a new ``--profile-startup`` option that only profiles the startup and configuration process (:issue:`7648`). - Builtins return a maximum exit status of 255, rather than potentially overflowing. In particular, this affects ``exit``, ``return``, ``functions --query``, and ``set --query`` (:issue:`7698`, :issue:`7702`). - It is no longer an error to run builtin with closed stdin. For example ``count <&-`` now prints 0, instead of failing. +- Blocks, functions, and builtins no longer permit redirecting to fds above 2. For example ``echo hello >&5`` is now an error. External commands may redirect to any fd below 10. Interactive improvements diff --git a/doc_src/index.rst b/doc_src/index.rst index 58079a835..49a60ef84 100644 --- a/doc_src/index.rst +++ b/doc_src/index.rst @@ -269,6 +269,8 @@ Any arbitrary file descriptor can used in a redirection by prefixing the redirec For example, ``echo hello 2> output.stderr`` writes the standard error (file descriptor 2) to ``output.stderr``. +It is an error to redirect a builtin, function, or block to a file descriptor above 2. However this is supported for external commands. + .. [#] Previous versions of fish also allowed specifying this as ``^DESTINATION``, but that made another character special so it was deprecated and will be removed in the future. See :ref:`feature flags`. .. _pipes: diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 09638b7a5..225edb494 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -900,14 +900,19 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( return arg_result; } + // Determine the process type. + process_type = process_type_for_command(statement, cmd); + + // Only external commands and exec may redirect arbitrary fds. + bool allow_high_fds = + (process_type == process_type_t::external || process_type == process_type_t::exec); + // The set of IO redirections that we construct for the process. - auto reason = this->determine_redirections(statement.args_or_redirs, &redirections); + auto reason = + this->determine_redirections(statement.args_or_redirs, allow_high_fds, &redirections); if (reason != end_execution_reason_t::ok) { return reason; } - - // Determine the process type. - process_type = process_type_for_command(statement, cmd); } // Populate the process. @@ -980,7 +985,8 @@ end_execution_reason_t parse_execution_context_t::expand_arguments_from_nodes( } end_execution_reason_t parse_execution_context_t::determine_redirections( - const ast::argument_or_redirection_list_t &list, redirection_spec_list_t *out_redirections) { + const ast::argument_or_redirection_list_t &list, bool allow_high_fds, + redirection_spec_list_t *out_redirections) { // Get all redirection nodes underneath the statement. for (const ast::argument_or_redirection_t &arg_or_redir : list) { if (!arg_or_redir.is_redirection()) continue; @@ -1008,10 +1014,18 @@ end_execution_reason_t parse_execution_context_t::determine_redirections( redirection_spec_t spec{oper->fd, oper->mode, std::move(target)}; // Validate this spec. - if (spec.mode == redirection_mode_t::fd && !spec.is_close() && !spec.get_target_as_fd()) { - const wchar_t *fmt = - _(L"Requested redirection to '%ls', which is not a valid file descriptor"); - return report_error(STATUS_INVALID_ARGS, redir_node, fmt, spec.target.c_str()); + if (spec.mode == redirection_mode_t::fd && !spec.is_close()) { + maybe_t target_fd = spec.get_target_as_fd(); + if (!target_fd.has_value()) { + // Like `cmd >&gibberish`. + const wchar_t *fmt = + _(L"Requested redirection to '%ls', which is not a valid file descriptor"); + return report_error(STATUS_INVALID_ARGS, redir_node, fmt, spec.target.c_str()); + } else if (*target_fd > STDERR_FILENO && !allow_high_fds) { + // Like `echo foo 2>&5`. Don't allow internal procs to write to internal fish fds. + const wchar_t *fmt = _(L"Redirection to fd %d is only valid for external commands"); + return report_error(STATUS_INVALID_ARGS, redir_node, fmt, *target_fd); + } } out_redirections->push_back(std::move(spec)); @@ -1066,7 +1080,8 @@ end_execution_reason_t parse_execution_context_t::populate_block_process( assert(args_or_redirs && "Should have args_or_redirs"); redirection_spec_list_t redirections; - auto reason = this->determine_redirections(*args_or_redirs, &redirections); + auto reason = + this->determine_redirections(*args_or_redirs, false /* no high fds */, &redirections); if (reason == end_execution_reason_t::ok) { proc->type = process_type_t::block_node; proc->block_node_source = pstree; diff --git a/src/parse_execution.h b/src/parse_execution.h index 5220d7937..4d5a0369b 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -130,7 +130,11 @@ class parse_execution_context_t { globspec_t glob_behavior); // Determines the list of redirections for a node. + // If \p allow_high_fds is false, then report an error for an fd redirection other than to + // in/out/err. This is set when constructing an internal process and prevents writing random + // data to internal fish fds. end_execution_reason_t determine_redirections(const ast::argument_or_redirection_list_t &list, + bool allow_high_fds, redirection_spec_list_t *out_redirections); end_execution_reason_t run_1_job(const ast::job_t &job, const block_t *associated_block); diff --git a/tests/checks/fds.fish b/tests/checks/fds.fish index c3b606a2c..c2a60bb1f 100644 --- a/tests/checks/fds.fish +++ b/tests/checks/fds.fish @@ -23,6 +23,9 @@ $helper print_fds &2 +# CHECK: 0 1 2 5 + # This attempts to trip a case where the file opened in fish # has the same fd as the redirection. In this case, the dup2 # does not clear the CLO_EXEC bit. diff --git a/tests/checks/redirect.fish b/tests/checks/redirect.fish index 50222c264..7c5d112cf 100644 --- a/tests/checks/redirect.fish +++ b/tests/checks/redirect.fish @@ -100,10 +100,35 @@ echo $status read abc <&- #CHECKERR: read: stdin is closed +# This one should output nothing. +echo derp >&- +# Verify that builtins, blocks, and functions may not write to arbitrary fds. +echo derp >&12 +#CHECKERR: {{.*}} Redirection to fd 12 is only valid for external commands +#CHECKERR: echo derp >&12 +#CHECKERR: ^ + +begin + echo derp +end <&42 +#CHECKERR: {{.*}} Redirection to fd 42 is only valid for external commands +#CHECKERR: end <&42 +#CHECKERR: ^ + +outnerr 2>&7 +#CHECKERR: {{.*}} Redirection to fd 7 is only valid for external commands +#CHECKERR: outnerr 2>&7 +#CHECKERR: ^ + +# Redirection to 0, 1, 2 is allowed. We don't test 0 since writing to stdin is weird and unpredictable. +echo hooray1 >&1 +echo hooray2 >&2 +#CHECK: hooray1 +#CHECKERR: hooray2 # "Verify that pipes don't conflict with fd redirections" -# This code is very similar to eval. We go over a bunch of fads +# This code is very similar to eval. We go over a bunch of fds # to make it likely that we will nominally conflict with a pipe # fish is supposed to detect this case and dup the pipe to something else echo "/bin/echo pipe 3 <&3 3<&-" | source 3<&0 @@ -113,9 +138,6 @@ echo "/bin/echo pipe 6 <&6 6<&-" | source 6<&0 echo "/bin/echo pipe 7 <&7 7<&-" | source 7<&0 echo "/bin/echo pipe 8 <&8 8<&-" | source 8<&0 echo "/bin/echo pipe 9 <&9 9<&-" | source 9<&0 -echo "/bin/echo pipe 10 <&10 10<&-" | source 10<&0 -echo "/bin/echo pipe 11 <&11 11<&-" | source 11<&0 -echo "/bin/echo pipe 12 <&12 12<&-" | source 12<&0 #CHECK: pipe 3 #CHECK: pipe 4 #CHECK: pipe 5 @@ -123,6 +145,3 @@ echo "/bin/echo pipe 12 <&12 12<&-" | source 12<&0 #CHECK: pipe 7 #CHECK: pipe 8 #CHECK: pipe 9 -#CHECK: pipe 10 -#CHECK: pipe 11 -#CHECK: pipe 12