8000 Saturate return value in builtin_set_query by ethulhu · Pull Request #7698 · fish-shell/fish-shell · GitHub
[go: up one dir, main page]
Skip to content

Saturate return value in builtin_set_query#7698

Closed
ethulhu wants to merge 1 commit intofish-shell:masterfrom
ethulhu:set-query-overflow
Closed

Saturate return value in builtin_set_query#7698
ethulhu wants to merge 1 commit intofish-shell:masterfrom
ethulhu:set-query-overflow

Conversation

@ethulhu
Copy link
@ethulhu ethulhu commented Feb 8, 2021

Description

builtin_set_query returns the number of missing variables. Because the return value passed to the shell is an 8-bit unsigned integer, if the number of missing variables is a multiple of 256, prior to this PR it would overflow to 0.

This PR saturates the return value at 255 if there are more than 255 missing variables.

This issue also seems to apply to functions --query and probably others; this PR only fixes set --query for now, but could be expanded to fix others.

Currently, it is fixed with an ad-hoc if-statement, but it could be fixed with some shared utility function like int saturate_error_count_to_255(int):

/* In builtin_set.cpp */
int builtin_set_query(…) {
    …
    return saturate_error_count_to_255(retval);
}

/* In builtin_functions.cpp */
int builtin_functions_query(…) {
    …
    return saturate_error_count_to_255(retval);
}

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

faho
faho requested changes 8000 Feb 8, 2021
View reviewed changes
@faho
Copy link
Member
faho commented Feb 8, 2021

Currently, it is fixed with an ad-hoc if-statement, but it could be fixed with some shared utility function like int saturate_error_count_to_255(int):

Considering that the shared utility function would be about the same amount of code I don't think it helps.

Now, if the builtin return type could be some saturating one or something that might be of use, but tbh it still feels like overkill.

@faho faho added the bug Something that's not working as intended label Feb 8, 2021
@faho faho added this to the fish 3.3.0 milestone Feb 8, 2021
@ethulhu
Saturate return value in builtin_set_query
b280ede 
builtin_set_query returns the number of missing variables. Because the
return value passed to the shell is an 8-bit unsigned integer, if the
number of missing variables is a multiple of 256, it would overflow to 0.

This commit saturates the return value at 255 if there are more than 255
missing variables.
@ethulhu ethulhu force-pushed the set-query-overflow branch from c807f7c to b280ede Compare February 8, 2021 19:12
@faho
Copy link
Member
faho commented Feb 8, 2021

Merged as 6dd6a57 (with the CHANGELOG changed to 3.2.0).

Thanks!

@faho faho closed this Feb 8, 2021
@faho faho modified the milestones: fish 3.3.0, fish 3.2.0 Feb 8, 2021
@ethulhu
Copy link
Author
ethulhu commented Feb 8, 2021

Currently, it is fixed with an ad-hoc if-statement, but it could be fixed with some shared utility function like int saturate_error_count_to_255(int):

Considering that the shared utility function would be about the same amount of code I don't think it helps.

Now, if the builtin return type could be some saturating one or something that might be of use, but tbh it still feels like overkill.

It could also be implemented higher up the callstack, e.g. in builtin_run() or proc_status_t, to saturate any return status higher than 255 from any function.

For example, putting the saturation check into proc_status_t::from_exit_code(int) passes all tests:

    /// Construct directly from an exit code.
    static proc_status_t from_exit_code(int ret) {
        // Some paranoia.
        constexpr int zerocode = w_exitcode(0, 0);
        static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited");

        if (ret > 255) {
            ret = 255;
        }
        return proc_status_t(w_exitcode(ret, 0 /* sig */));
    }

@krobelus
Copy link
Contributor
krobelus commented Feb 8, 2021

I'd either move the saturation logic to builtin_run (because it only happens with builtins, not with external commands), or apply a similar fix to other callers like functions -q.

Maybe adding an assert(ret < 256) as well is worth it? Hard to tell.

@ethulhu
Copy link
Author
ethulhu commented Feb 8, 2021

I'd either move the saturation logic to builtin_run (because it only happens with builtins, not with external commands), or apply a similar fix to other callers like functions -q.

Maybe adding an assert(ret < 256) as well is worth it? Hard to tell.

Like this? master...ethulhu:saturate-all-builtins (not a PR because I've not changed any documentation yet)

@krobelus
Copy link
Contributor
krobelus commented Feb 9, 2021

Looking through the documented exit codes, I think we have four affected builtins:

  • set -q
  • functions -q
  • exit
  • return

exit and return should probably print an error when the argument is > 255.
I think we should change the behavior of set -q and functions -q to always exit with 0 or 1. I doubt that anyone is doing arithmetic on those exit codes?

Anyway, your change looks good and is maybe safer, so feel free to go for it.

@ethulhu ethulhu mentioned this pull request Feb 9, 2021
Closed
3 tasks
@ethulhu
Copy link
Author
ethulhu commented Feb 9, 2021

exit and return should probably print an error when the argument is > 255.
I think we should change the behavior of set -q and functions -q to always exit with 0 or 1. I doubt that anyone is doing arithmetic on those exit codes?

Anyway, your change looks good and is maybe safer, so feel free to go for it.

I agree that exit and return should probably raise an error on an invalid value, but also agree that saturating return values to 255 is more compatible with the previous behavior and less likely to cause mysterious errors in user code.

I've filled in the branch I linked earlier, and made it into PR #7702.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
faho referenced this pull request Nov 12, 2021
@floam
Long march towards more structured text
ac1df31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@faho faho faho requested changes

Assignees

No one assigned

Labels

bug Something that's not working as intended

Projects

None yet

Milestone

fish 3.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ethulhu @faho @krobelus
0