h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-05 00:01:15 -03:00
Code Issues Packages Projects Releases Wiki Activity

Make maybe_t safer against accidental misuse

Browse Source 
Closes #9240.

Squash of the following commits (in reverse-chronological order):

commit 03b5cab3dc40eca9d50a9df07a8a32524338a807
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date:   Sun Sep 25 15:09:04 2022 -0500

    Handle differently declared posix_spawnxxx_t on macOS

    On macOS, posix_spawnattr_t and posix_spawn_file_actions_t are declared as void
    pointers, so we can't use maybe_t's bool operator to test if it has a value.

commit aed83b8bb308120c0f287814d108b5914593630a
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date:   Sun Sep 25 14:48:46 2022 -0500

    Update maybe_t tests to reflect dynamic bool conversion

    maybe_t<T> is now bool-convertible only if T _isn't_ already bool-convertible.

commit 2b5a12ca97b46f96b1c6b56a41aafcbdb0dfddd6
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date:   Sun Sep 25 14:34:03 2022 -0500

    Make maybe_t a little harder to misuse

    We've had a few bugs over the years stemming from accidental misuse of maybe_t
    with bool-convertible types. This patch disables maybe_t's bool operator if the
    type T is already bool convertible, forcing the (barely worth mentioning) need
    to use maybe_t::has_value() instead.

    This patch both removes maybe_t's bool conversion for bool-convertible types and
    updates the existing codebase to use the explicit `has_value()` method in place
    of existing implicit bool conversions.
This commit is contained in:
Mahmoud Al-Qudsi
2022-10-08 11:56:38 -05:00
parent 485873b19b
commit 85d4834b35
20 changed files with 77 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/builtin.cpp
View File

@@ -460,7 +460,7 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre
// Resolve our status code.
// If the builtin itself produced an error, use that error.
// Otherwise use any errors from writing to out and writing to err, in that order.
int code = builtin_ret ? *builtin_ret : 0;
int code = builtin_ret.has_value() ? *builtin_ret : 0;
if (code == 0) code = out_ret;
if (code == 0) code = err_ret;

2
src/builtins/disown.cpp
View File

@@ -28,7 +28,7 @@ static void disown_job(const wchar_t *cmd, io_streams_t &streams, job_t *j) {
// Stopped disowned jobs must be manually signaled; explain how to do so.
auto pgid = j->get_pgid();
if (j->is_stopped()) {
if (pgid) killpg(*pgid, SIGCONT);
if (pgid.has_value()) killpg(*pgid, SIGCONT);
const wchar_t *fmt =
_(L"%ls: job %d ('%ls') was stopped and has been signalled to continue.\n");
streams.err.append_format(fmt, cmd, j->job_id(), j->command_wcstr());

7
src/builtins/jobs.cpp
View File

@@ -42,8 +42,11 @@ static double cpu_use(const job_t *j) {
/// Print information about the specified job.
static void builtin_jobs_print(const job_t *j, int mode, int header, io_streams_t &streams) {
int pgid = INVALID_PID;
if (auto job_pgid = j->get_pgid()) {
pgid = *job_pgid;
{
auto job_pgid = j->get_pgid();
if (job_pgid.has_value()) {
pgid = *job_pgid;
}
}
wcstring out;

4
src/builtins/path.cpp
View File

@@ -671,7 +671,7 @@ static int path_extension(parser_t &parser, io_streams_t &streams, int argc, con
while (const wcstring *arg = aiter.nextstr()) {
auto pos = find_extension(*arg);
if (!pos) {
if (!pos.has_value()) {
// If there is no extension the extension is empty.
// This is unambiguous because we include the ".".
path_out(streams, opts, L"");
@@ -702,7 +702,7 @@ static int path_change_extension(parser_t &parser, io_streams_t &streams, int ar
auto pos = find_extension(*arg);
wcstring ext;
if (!pos) {
if (!pos.has_value()) {
ext = *arg;
} else {
ext = arg->substr(0, *pos);

7
src/builtins/string.cpp
View File

@@ -224,7 +224,7 @@ static size_t width_without_escapes(const wcstring &ins, size_t start_pos = 0) {
size_t pos = start_pos;
while ((pos = ins.find('\x1B', pos)) != std::string::npos) {
auto len = escape_code_length(ins.c_str() + pos);
if (len) {
if (len.has_value()) {
auto sub = ins.substr(pos, *len);
for (auto c : sub) {
auto w = fish_wcwidth_visible(c);
@@ -1217,7 +1217,8 @@ static maybe_t<wcstring> interpret_escapes(const wcstring &arg) {
const wchar_t *end = cursor + arg.size();
while (cursor < end) {
if (*cursor == L'\\') {
if (auto escape_len = read_unquoted_escape(cursor, &result, true, false)) {
auto escape_len = read_unquoted_escape(cursor, &result, true, false);
if (escape_len.has_value()) {
cursor += *escape_len;
} else {
// Invalid escape.
@@ -1725,7 +1726,7 @@ static int string_shorten(parser_t &parser, io_streams_t &streams, int argc, con
size_t totallen = 0;
while (l[pos + totallen] == L'\x1B') {
auto len = escape_code_length(l.c_str() + pos + totallen);
if (!len) break;
if (!len.has_value()) break;
totallen += *len;
}
return totallen;

2
src/common.cpp
View File

@@ -1370,7 +1370,7 @@ static bool unescape_string_internal(const wchar_t *const input, const size_t in
// appending INTERNAL_SEPARATORs, so we have to handle them specially.
auto escape_chars = read_unquoted_escape(
input + input_position, &result, allow_incomplete, unescape_special);
if (!escape_chars) {
if (!escape_chars.has_value()) {
// A none() return indicates an error.
errored = true;
} else {

8
src/complete.cpp
View File

@@ -1308,7 +1308,7 @@ cleanup_t completer_t::apply_var_assignments(const wcstring_list_t &var_assignme
const block_t *block = ctx.parser->push_block(block_t::variable_assignment_block());
for (const wcstring &var_assign : var_assignments) {
maybe_t<size_t> equals_pos = variable_assignment_equals_pos(var_assign);
assert(equals_pos && "All variable assignments should have equals position");
assert(equals_pos.has_value() && "All variable assignments should have equals position");
const wcstring variable_name = var_assign.substr(0, *equals_pos);
const wcstring expression = var_assign.substr(*equals_pos + 1);
@@ -1395,7 +1395,7 @@ void completer_t::walk_wrap_chain(const wcstring &cmd, const wcstring &cmdline,
size_t wrapped_command_offset_in_wt = wcstring::npos;
while (auto tok = tokenizer.next()) {
wcstring tok_src = tok->get_source(wt);
if (variable_assignment_equals_pos(tok_src)) {
if (variable_assignment_equals_pos(tok_src).has_value()) {
ad->var_assignments->push_back(std::move(tok_src));
} else {
wrapped_command_offset_in_wt = tok->offset;
@@ -1542,7 +1542,7 @@ void completer_t::perform_for_commandline(wcstring cmdline) {
for (const tok_t &tok : tokens) {
if (tok.location_in_or_at_end_of_source_range(cursor_pos)) break;
wcstring tok_src = tok.get_source(cmdline);
if (!variable_assignment_equals_pos(tok_src)) break;
if (!variable_assignment_equals_pos(tok_src).has_value()) break;
var_assignments.push_back(std::move(tok_src));
}
tokens.erase(tokens.begin(), tokens.begin() + var_assignments.size());
@@ -1585,7 +1585,7 @@ void completer_t::perform_for_commandline(wcstring cmdline) {
if (cmd_tok.location_in_or_at_end_of_source_range(cursor_pos)) {
maybe_t<size_t> equal_sign_pos = variable_assignment_equals_pos(current_token);
if (equal_sign_pos) {
if (equal_sign_pos.has_value()) {
complete_param_expand(current_token, true /* do_file */);
return;
}

3
src/env.cpp
View File

@@ -1179,7 +1179,8 @@ mod_result_t env_stack_impl_t::set(const wcstring &key, env_mode_flags_t mode,
wcstring_list_t val) {
const query_t query(mode);
// Handle electric and read-only variables.
if (auto ret = try_set_electric(key, query, val)) {
auto ret = try_set_electric(key, query, val);
if (ret.has_value()) {
return mod_result_t{*ret};
}

12
src/exec.cpp
View File

@@ -415,9 +415,12 @@ static launch_result_t fork_child_for_process(const std::shared_ptr<job_t> &job,
if (p->leads_pgrp) {
job->group->set_pgid(p->pid);
}
if (auto pgid = job->group->get_pgid()) {
if (int err = execute_setpgid(p->pid, *pgid, is_parent)) {
report_setpgid_error(err, is_parent, *pgid, job.get(), p);
{
auto pgid = job->group->get_pgid();
if (pgid.has_value()) {
if (int err = execute_setpgid(p->pid, *pgid, is_parent)) {
report_setpgid_error(err, is_parent, *pgid, job.get(), p);
}
}
}
@@ -1115,7 +1118,8 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const io_chain_t &bl
// If exec_error then a backgrounded job would have been terminated before it was ever assigned
// a pgroup, so error out before setting last_pid.
if (!j->is_foreground()) {
if (maybe_t<pid_t> last_pid = j->get_last_pid()) {
maybe_t<pid_t> last_pid = j->get_last_pid();
if (last_pid.has_value()) {
parser.vars().set_one(L"last_pid", ENV_GLOBAL, to_string(*last_pid));
} else {
parser.vars().set_empty(L"last_pid", ENV_GLOBAL);

13
src/fish_tests.cpp
View File

@@ -2915,7 +2915,7 @@ static bool run_one_test_test(int expected, const wcstring_list_t &lst, bool bra
maybe_t<int> result = builtin_test(parser, streams, cargv.get());
if (result != expected) {
std::wstring got = result ? std::to_wstring(result.value()) : L"nothing";
std::wstring got = result.has_value() ? std::to_wstring(result.value()) : L"nothing";
err(L"expected builtin_test() to return %d, got %s", expected, got.c_str());
}
return result == expected;
@@ -5895,7 +5895,9 @@ static void run_one_string_test(const wchar_t *const *argv_raw, int expected_rc,
args.resize(args.size() - 1);
if (rc != expected_rc) {
std::wstring got = rc ? std::to_wstring(rc.value()) : L"nothing";
// The comparison above would have panicked if rc didn't have a value, so it's safe to
// assume it has one here:
std::wstring got = std::to_wstring(rc.value());
err(L"Test failed on line %lu: [%ls]: expected return code %d but got %s", __LINE__,
args.c_str(), expected_rc, got.c_str());
} else if (outs.contents() != expected_out) {
@@ -6371,7 +6373,8 @@ static void test_illegal_command_exit_code() {
void test_maybe() {
say(L"Testing maybe_t");
do_test(!bool(maybe_t<int>()));
// maybe_t<T> bool conversion is only enabled for non-bool-convertible T types
do_test(!bool(maybe_t<wcstring>()));
maybe_t<int> m(3);
do_test(m.has_value());
do_test(m.value() == 3);
@@ -6389,8 +6392,8 @@ void test_maybe() {
do_test(m != maybe_t<int>());
do_test(maybe_t<int>() == none());
do_test(!maybe_t<int>(none()).has_value());
m = none();
do_test(!bool(m));
maybe_t<wcstring> n = none();
do_test(!bool(n));
maybe_t<std::string> m2("abc");
do_test(!m2.missing_or_empty());

5
src/highlight.cpp
View File

@@ -1026,7 +1026,8 @@ void highlighter_t::visit(const ast::argument_t &arg, bool cmd_is_cd, bool optio
void highlighter_t::visit(const ast::variable_assignment_t &varas) {
color_as_argument(varas);
// Highlight the '=' in variable assignments as an operator.
if (auto where = variable_assignment_equals_pos(varas.source(this->buff))) {
auto where = variable_assignment_equals_pos(varas.source(this->buff));
if (where.has_value()) {
size_t equals_loc = varas.source_range().start + *where;
this->color_array.at(equals_loc) = highlight_role_t::operat;
auto var_name = varas.source(this->buff).substr(0, *where);
@@ -1048,7 +1049,7 @@ void highlighter_t::visit(const ast::decorated_statement_t &stmt) {
if (!this->io_ok) {
// We cannot check if the command is invalid, so just assume it's valid.
is_valid_cmd = true;
} else if (variable_assignment_equals_pos(*cmd)) {
} else if (variable_assignment_equals_pos(*cmd).has_value()) {
is_valid_cmd = true;
} else {
// Check to see if the command is valid.

7
src/history.cpp
View File

@@ -582,7 +582,9 @@ void history_impl_t::populate_from_file_contents() {
old_item_offsets.clear();
if (file_contents) {
size_t cursor = 0;
while (auto offset = file_contents->offset_of_next_item(&cursor, boundary_timestamp)) {
maybe_t<size_t> offset;
while ((offset =
file_contents->offset_of_next_item(&cursor, boundary_timestamp)).has_value()) {
// Remember this item.
old_item_offsets.push_back(*offset);
}
@@ -725,7 +727,8 @@ bool history_impl_t::rewrite_to_temporary_file(int existing_fd, int dst_fd) cons
// old file contents).
if (auto local_file = history_file_contents_t::create(existing_fd)) {
size_t cursor = 0;
while (auto offset = local_file->offset_of_next_item(&cursor, 0)) {
maybe_t<size_t> offset;
while ((offset = local_file->offset_of_next_item(&cursor, 0)).has_value()) {
// Try decoding an old item.
history_item_t old_item = local_file->decode_item(*offset);

10
src/maybe.h
View File

@@ -134,8 +134,14 @@ class maybe_t : private maybe_detail::conditionally_copyable_t<T> {
// return whether the receiver contains a value.
bool has_value() const { return impl_.filled; }
// bool conversion indicates whether the receiver contains a value.
explicit operator bool() const { return impl_.filled; }
// A bool operator as a shortcut to test if the maybe_t has a value.
// Not enabled if the type T is already bool-convertible to prevent accidental misuse,
// otherwise the "typename std::enable_if<....>::type" evaluates to bool, giving us a definition
// of `explicit operator bool() const { ... }`
template <typename U = T> explicit operator
typename std::enable_if<!std::is_convertible<U, bool>::value, bool>::type() const {
return impl_.filled;
}
// The default constructor constructs a maybe with no value.
maybe_t() = default;

5
src/parse_execution.cpp
View File

@@ -1021,7 +1021,8 @@ 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()) {
if (spec.mode == redirection_mode_t::fd && !spec.is_close() &&
!spec.get_target_as_fd().has_value()) {
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());
@@ -1095,7 +1096,7 @@ end_execution_reason_t parse_execution_context_t::apply_variable_assignments(
for (const ast::variable_assignment_t &variable_assignment : variable_assignment_list) {
const wcstring &source = get_source(variable_assignment);
auto equals_pos = variable_assignment_equals_pos(source);
assert(equals_pos);
assert(equals_pos.has_value());
const wcstring variable_name = source.substr(0, *equals_pos);
const wcstring expression = source.substr(*equals_pos + 1);
completion_list_t expression_expanded;

2
src/parse_tree.cpp
View File

@@ -53,7 +53,7 @@ wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring
case parse_error_bare_variable_assignment: {
wcstring assignment_src = src.substr(this->source_start, this->source_length);
maybe_t<size_t> equals_pos = variable_assignment_equals_pos(assignment_src);
assert(equals_pos);
assert(equals_pos.has_value());
wcstring variable = assignment_src.substr(0, *equals_pos);
wcstring value = assignment_src.substr(*equals_pos + 1);
append_format(result, ERROR_BAD_COMMAND_ASSIGN_ERR_MSG, variable.c_str(),

15
src/postfork.cpp
View File

@@ -256,10 +256,10 @@ bool posix_spawner_t::check_fail(int err) {
}
posix_spawner_t::~posix_spawner_t() {
if (attr_) {
if (attr_.has_value()) {
posix_spawnattr_destroy(this->attr());
}
if (actions_) {
if (actions_.has_value()) {
posix_spawn_file_actions_destroy(this->actions());
}
}
@@ -281,10 +281,13 @@ posix_spawner_t::posix_spawner_t(const job_t *j, const dup2_list_t &dup2s) {
// desired_pgid tracks the pgroup for the process. If it is none, the pgroup is left unchanged.
// If it is zero, create a new pgroup from the pid. If it is >0, join that pgroup.
maybe_t<pid_t> desired_pgid = none();
if (auto pgid = j->group->get_pgid()) {
desired_pgid = *pgid;
} else if (j->processes.front()->leads_pgrp) {
desired_pgid = 0;
{
auto pgid = j->group->get_pgid();
if (pgid.has_value()) {
desired_pgid = *pgid;
} else if (j->processes.front()->leads_pgrp) {
desired_pgid = 0;
}
}
// Set the handling for job control signals back to the default.

8
src/proc.cpp
View File

@@ -123,7 +123,8 @@ bool job_t::posts_job_exit_events() const {
}
bool job_t::signal(int signal) {
if (auto pgid = group->get_pgid()) {
auto pgid = group->get_pgid();
if (pgid.has_value()) {
if (killpg(*pgid, signal) == -1) {
char buffer[512];
snprintf(buffer, 512, "killpg(%d, %s)", *pgid, strsignal(signal));
@@ -475,7 +476,8 @@ static void generate_job_exit_events(const job_ref_t &j, std::vector<event_t> *o
if (!j->from_event_handler() || !j->is_foreground()) {
// job_exit events.
if (j->posts_job_exit_events()) {
if (auto last_pid = j->get_last_pid()) {
auto last_pid = j->get_last_pid();
if (last_pid.has_value()) {
out_evts->push_back(event_t::job_exit(*last_pid, j->internal_job_id));
}
}
@@ -976,7 +978,7 @@ void hup_jobs(const job_list_t &jobs) {
pid_t fish_pgrp = getpgrp();
for (const auto &j : jobs) {
auto pgid = j->get_pgid();
if (pgid && *pgid != fish_pgrp && !j->is_completed()) {
if (pgid.has_value() && *pgid != fish_pgrp && !j->is_completed()) {
if (j->is_stopped()) {
j->signal(SIGCONT);
}

2
src/reader.cpp
View File

@@ -1103,7 +1103,7 @@ wcstring combine_command_and_autosuggestion(const wcstring &cmdline,
/// Update the cursor position.
void reader_data_t::update_buff_pos(editable_line_t *el, maybe_t<size_t> new_pos) {
if (new_pos) {
if (new_pos.has_value()) {
el->set_position(*new_pos);
}
size_t buff_pos = el->position();

2
src/screen.cpp
View File

@@ -268,7 +268,7 @@ size_t layout_cache_t::escape_code_length(const wchar_t *code) {
if (esc_seq_len) return esc_seq_len;
auto found = ::escape_code_length(code);
if (found) {
if (found.has_value()) {
this->add_escape_code(wcstring(code, *found));
esc_seq_len = *found;
}

10
src/tokenizer.cpp
View File

@@ -74,7 +74,7 @@ tok_t tokenizer_t::call_error(tokenizer_error_t error_type, const wchar_t *token
result.error = error_type;
result.offset = token_start - this->start;
// If we are passed a token_length, then use it; otherwise infer it from the buffer.
result.length = token_length ? *token_length : this->token_cursor - token_start;
result.length = token_length.has_value() ? *token_length : this->token_cursor - token_start;
result.error_offset_within_token = error_loc - token_start;
result.error_length = error_len;
return result;
@@ -110,7 +110,7 @@ static bool tok_is_string_character(wchar_t c, maybe_t<wchar_t> next) {
}
case L'&': {
if (!feature_test(features_t::ampersand_nobg_in_token)) return false;
bool next_is_string = next && tok_is_string_character(*next, none());
bool next_is_string = next.has_value() && tok_is_string_character(*next, none());
// Unlike in other shells, '&' is not special if followed by a string character.
return next_is_string;
}
@@ -658,7 +658,7 @@ maybe_t<tok_t> tokenizer_t::next() {
}
bool is_token_delimiter(wchar_t c, maybe_t<wchar_t> next) {
return c == L'(' || !tok_is_string_character(c, next);
return c == L'(' || !tok_is_string_character(c, std::move(next));
}
wcstring tok_command(const wcstring &str) {
@@ -668,7 +668,7 @@ wcstring tok_command(const wcstring &str) {
return {};
}
wcstring text = t.text_of(*token);
if (variable_assignment_equals_pos(text)) {
if (variable_assignment_equals_pos(text).has_value()) {
continue;
}
return text;
@@ -885,7 +885,7 @@ move_word_state_machine_t::move_word_state_machine_t(move_word_style_t syl)
void move_word_state_machine_t::reset() { state = 0; }
// Return the location of the equals sign, or npos if the string does
// Return the location of the equals sign, or none if the string does
// not look like a variable assignment like FOO=bar. The detection
// works similar as in some POSIX shells: only letters and numbers qre
// allowed on the left hand side, no quotes or escaping.