Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saturate return value in builtin_set_query #7698

Closed
wants to merge 1 commit into from

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

tests/checks/set.fish Outdated Show resolved Hide resolved
@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
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.
@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
Member

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
Member

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
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants