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

Fix comparison of RawVals of differing types and objectness #767

Merged
merged 4 commits into from
May 1, 2023

Conversation

brson
Copy link
Contributor

@brson brson commented Apr 19, 2023

Edit: I have posted a prerequisite to this patch in #773

What

Make comparison of RawVals agree with comparison of ScVals in the case where one value is an object and one value is a smallval, and they are not the same ScValType.

Also fix an unsoundness in RawVal::get_tag due to incorrect assumptions in an unsafe cast.

Why

Per #743, comparison operations for RawVals and ScVals must agree.

The final fallthrough case of Host::obj_cmp does a comparison based on the Tag discriminant, and the Tag discriminants do not have the same ordering as the ScVal discriminants. This patch adds a method Tag::get_scval_type, and changes obj_cmp to use that instead.

This adds a thorough test case which does an exhaustive conversion between u8s and tags, and an exhaustive match on Tags. The exhaustive match triggered a segfault due to an unsound cast from u8 to Tag in RawVal::get_tag. That function assumed that Tag's discriminants were contiguous between the "marker" discriminants ObjectCodeLowerBound and ObjectCodeUpperBound, which was not true. get_tag looks like

    #[inline(always)]
    pub const fn get_tag(self) -> Tag {
        let tag = self.get_tag_u8();
        const A: u8 = Tag::SmallCodeUpperBound as u8;
        const B: u8 = Tag::ObjectCodeLowerBound as u8;
        const C: u8 = Tag::ObjectCodeUpperBound as u8;
        if !((tag < A) || (B < tag && tag < C)) {
            return Tag::Bad;
        }
        // Transmuting an integer to an enum is UB if outside the defined enum
        // value set, so we need to test above to be safe. Note that it's ok for
        // `get_tag` here to be a _little_ slow since it's not called in a lot
        // of small/tight paths, only when doing a switch-based comparison. Most
        // small paths call `has_tag` which tests a _known_ enum case against
        // the tag byte, and therefore doesn't need the range check.
        unsafe { ::core::mem::transmute(tag) }
    }

In safe code it may not be possible to trigger the unsoundness - it may require unsafely constructing RawVal. It is not clear to me that contracts can find this code path, but it's probably best to fix it.

I changed the discriminants of Tag to make RawVal::get_tag always correct, and added docs, but the code is brittle to changes. I considered how to make it less error prone and add tests, but could not come up with a better test case than the one that is already in this patch.

This adds a dev-dependency on itertools for the cartesian_product method.

Known limitations

na

@brson
Copy link
Contributor Author

brson commented Apr 19, 2023

Actually the test here is not exhaustive in the conversion of u8s to Tags. There could be a better test of RawVal::get_tag, but I'll wait for some review before thinking about it more.

@brson
Copy link
Contributor Author

brson commented Apr 19, 2023

I can reproduce the CI failures. Seems like I've broken something.

@brson
Copy link
Contributor Author

brson commented Apr 22, 2023

The CI failure was because changing Tag discriminants is ABI-breaking and requires rebuilding the soroban-test-wasms. I tried, but the wasm-workspace does not build presently. Instead I wrote a testcase and simple, possibly temporary, fix and posted a separate pr #773

@brson brson force-pushed the obj-small-cmp branch 3 times, most recently from 1b96a76 to 1461a54 Compare April 28, 2023 20:59
@brson
Copy link
Contributor Author

brson commented Apr 28, 2023

Rebased. Still needs #767 to land first.

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be panic-free

soroban-env-common/src/raw_val.rs Outdated Show resolved Hide resolved
@brson
Copy link
Contributor Author

brson commented Apr 29, 2023

I removed the panics from Tag::get_scval_type. I also realized that I had other comparison tests located in another module, so I moved the tests from this PR there.

@brson
Copy link
Contributor Author

brson commented Apr 29, 2023

Also did a rebase to pick up the commits from #773

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@graydon graydon enabled auto-merge (squash) May 1, 2023 18:42
@graydon graydon merged commit c171b72 into stellar:main May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants