Guard against overflow in codemap::span_to_lines.

[pnkfelix]

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.

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[lilyball]

Speaking of overflowing in slice, I think we still have other overflow possibilities. It appears as though slice was written to work properly in the case of zero-sized slices where s.as_ptr() + s.len() wrapped around (specifically, it doesn't seem to rely in the iterators that end > ptr, and it explicitly handles the case of wrapping around during iteration by always yielding &mut *(1 as *mut _) as the iterator value for zero-sized elements (because yielding null would be interpreted as None instead of as Some(_)). But all this code was written with wrapping arithmetic in mind. So there's code that does things like ptr as usize + len(), which might trigger an overflow check.

I'll file a ticket about it. (Update: Filed as #25016)

[pnkfelix]

r? @huonw

[huonw]

Could you add a comment about why one can't just use slice_offset!/slice_add_offset! with -1?

[pnkfelix]

will do.

[huonw]

This change will 'disguise' these bugs, won't it? (i.e. instead of an error/ICE, one will get (unprintable span))

Maybe it could be phrased as (internal error: invalid span) or something that suggests it's a bug more strongly?

[pnkfelix]

@kballard wrote:

This could be done simpler just by using wrapping arithmetic.

By the way, I did see this feedback, and as a result, I considered doing this. But I think the formulation I came up with is more self-documenting, and should be just as efficient when compiled without -C debug-assertions.

(By "self-documenting," I guess I mean that this is not a case where modular arithmetic inherently applies -- instead, using wrapping_add would take advantage of one special property of the twos-complement representation, but other potential overflow bugs would then be masked...)

I just wanted to make sure I addressed this note. :)

[pnkfelix]

@huonw wrote:

Maybe it could be phrased as (internal error: invalid span) or something that suggests it's a bug more strongly?

Yeah, I also had thought a stronger message would be warranted. (Should I go so far as to find some way to encode "please report this as a bug" into the output as well?)

[lilyball]

What other potential overflow bugs would be masked? My understanding is this code is explicitly supposed to be tolerant of wrapping because it still maintains the desired invariants (namely, that the end can be reached from the start by incrementing the correct number of times). So it's not an accident that two's-complement works, it's explicit design.

[pnkfelix]

@kballard

What other potential overflow bugs would be masked?

eh, the only examples I can think of offhand involve overflowing usize::MAX, but its really a weak strawman.

[pnkfelix]

@huonw any further review feedback?

[lilyball]

GitHub seems to have lost my previous comment about this. It may have been a commit comment instead of a diff comment but I thought GitHub still preserved those. In the interests of making the discussion actually make sense, what I had suggested was that we don't need a separate macro at all, we just need wrapping arithmetic, like

macro_rules! slice_offset {
    ($ptr:expr, $by:expr) => {{
        let ptr = $ptr;
        if size_from_ptr(ptr) == 0 {
            transmute((ptr as isize).wrapping_add($by))
        } else {
            ptr.offset($by)
        }
    }};
}

And FWIW I still believe this is the better approach. It more closely mimics the actual pointer arithmetic used for non-zero-sized types, it means none of the callers of the macro have to be updated, and it more closely matches the required changes for #25016 (I think we need to use wrapping addition in more places in this module in order to restore the pre-overflow-check behavior).

[pnkfelix]

I have kept the changes in the form presented here because I still believe this code is easier to read than that change that you suggest. (It is an entirely subjective opinion.)

I freely admit that I have not done the full review of the core::slice code, and thus I have no argument against the point you make about hypothetical fixes to #25016. If it comes to that, then we can revert back to a macro of the form you suggest when #25016 gets fixed.

For now, however, my main concern is getting make check working again on --enable-debug builds, and this was the most obviously correct way for me to go about doing it.

[pnkfelix]

@huonw review ping; I have now addressed your comments. Was there anything else you wanted here?

[huonw]

@bors r+

[bors]

📌 Commit 909e1d6 has been approved by huonw

[bors]

⌛ Testing commit 909e1d6 with merge 5858b66...

[bors]

⛄ The build was interrupted to prioritize another pull request.

[bors]

⌛ Testing commit 909e1d6 with merge cbc8a1c...

[bors]

💔 Test failed - auto-mac-64-opt

[pnkfelix]

sigh, my attempt to add an assertion to a test broke make check-pretty. I have now just removed that commit; the assertion (in commit 909e1d6 )wasn't that important.

[pnkfelix]

@bors r=huonw 939e4c9

[bors]

⌛ Testing commit 939e4c9 with merge a39d4fc...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt