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

vec::test_drain_range exposes overflow "bug" in slice code #24997

Closed
pnkfelix opened this issue Apr 30, 2015 · 1 comment · Fixed by #25013
Closed

vec::test_drain_range exposes overflow "bug" in slice code #24997

pnkfelix opened this issue Apr 30, 2015 · 1 comment · Fixed by #25013

Comments

@pnkfelix
Copy link
Member

While doing a full make check-stage1 on a rust build configured with --enable-debug, I encountered this failure in collectionstest:

---- vec::test_drain_range stdout ----
    thread 'vec::test_drain_range' panicked at 'arithmetic operation overflowed', /Users/fklock/Dev/Mozilla/rust-span_to_lines/src/libcore/slice.rs:639

That line is here in this macro:

macro_rules! slice_offset {
    ($ptr:expr, $by:expr) => {{
        let ptr = $ptr;
        if size_from_ptr(ptr) == 0 {
            transmute(ptr as usize + $by) // <=== this line
        } else {
            ptr.offset($by)
        }
    }};
}

According to preliminary analysis, this arises because of this code elsewhere in slice:

                        self.end = slice_offset!(self.end, -1);

The -1 there is going to get interpreted as a usize, and thus its going to cause the overflow check to fire.

Note that there is no actual bug being caught in this case; in fact the cases where it is signaling overflow (namely where self.end is nonzero) are exactly the only cases that should be treated as "working."


Anyway, an easy short-term fix for this is to introduce distinct slice_add_offset! and slice_sub_offset! macros that both take non-negative input values. I'm testing that change locally now.

@pnkfelix
Copy link
Member Author

(I guess if I were keeping track of false positives signaled by arithmetic overflow, then this would qualify as one.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 30, 2015
This overflow does not cause any problems; it just causes errors to be
signalled when compiling with `-C debug-assertions`.

Fix rust-lang#24997
pnkfelix added a commit to pnkfelix/rust that referenced this issue May 4, 2015
This overflow does not cause any problems; it just causes errors to be
signalled when compiling with `-C debug-assertions`.

Fix rust-lang#24997
bors added a commit that referenced this issue May 7, 2015
Guard against overflow in `codemap::span_to_lines`.

(Revised/expanded version of PR #24976)

Make `span_to_lines` to return a `Result`.

In `diagnostic`, catch `Err` from `span_to_lines` and print `"(unprintable span)"` instead.

----

There a number of recent issues that report the bug here.  See e.g. #24761 and #24954.

This change *might* fix them. However, that is *not* its main goal. The main goals are:

 1. Make it possible for callers to recover from an error here, and

 2. Insert a more conservative check, in that we are also checking that the files match up.

----

As a drive-by, fix #24997 , which was causing my attempts to `make check-stage1` on an `--enable-debug` build to fail.
bors added a commit that referenced this issue May 7, 2015
Guard against overflow in `codemap::span_to_lines`.

(Revised/expanded version of PR #24976)

Make `span_to_lines` to return a `Result`.

In `diagnostic`, catch `Err` from `span_to_lines` and print `"(unprintable span)"` instead.

----

There a number of recent issues that report the bug here.  See e.g. #24761 and #24954.

This change *might* fix them. However, that is *not* its main goal. The main goals are:

 1. Make it possible for callers to recover from an error here, and

 2. Insert a more conservative check, in that we are also checking that the files match up.

----

As a drive-by, fix #24997 , which was causing my attempts to `make check-stage1` on an `--enable-debug` build to fail.
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 a pull request may close this issue.

2 participants