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

Wrong size passed to free in issue-26709.rs run-pass test (yielding jemalloc assert fail) #27023

Closed
nagisa opened this issue Jul 13, 2015 · 8 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Jul 13, 2015

After ./configure --enable-rpath --disable-clang --disable-libcpp --enable-llvm-assertions --release-channel=dev --enable-debug --enable-optimize on linux, running tests will fail with:

---- [run-pass] run-pass/issue-26709.rs stdout ----

error: test run failed!
status: signal: 6
command: x86_64-unknown-linux-gnu/test/run-pass/issue-26709.stage1-x86_64-unknown-linux-gnu 
stdout:
------------------------------------------
 
------------------------------------------
stderr:
------------------------------------------
<jemalloc>: /home/nagisa/Documents/rust/rust/src/jemalloc/src/jemalloc.c:1885: Failed assertion: "usize == isalloc(ptr, config_prof)"
 
------------------------------------------
 
thread '[run-pass] run-pass/issue-26709.rs' panicked at 'explicit panic', /home/nagisa/Documents/rust/rust/src/compiletest/runtest.rs:1490

I should make it clear I have no minimal reproduction and am not even sure whether configure flags. my system configuration or any local state have anything to do with reproducability.

@elinorbgr
Copy link
Contributor

I also reproduce this running make check-stage1-rpass on my 64bits linux with this configure:

./configure --llvm-root=/usr/ --enable-debug --disable-docs --enable-optimize

@steveklabnik steveklabnik added A-testsuite Area: The testsuite used to check the correctness of rustc A-build labels Jul 16, 2015
@pnkfelix
Copy link
Member

I can reproduce this as well on my Mac. (After applying #27139 to get around debuginfo issues, I then hit this bug; configure flags --enable-debug --enable-optimize --enable-ccache --enable-clang --disable-optimize-tests --disable-llvm-assertions --disable-debuginfo)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 20, 2015
…s needed for run-pass tests.

I still cannot do complete run-pass run in `--enable-debug` (*) build
due to rust-lang#27023.

But these bits are certainly necessary.

(*) (even after disabling debuginfo, see rust-lang#27139)
@pnkfelix pnkfelix changed the title issue-26709.rs run-pass test fails with jemalloc assertion jemalloc assert fail in issue-26709.rs run-pass test Jul 20, 2015
@pnkfelix pnkfelix added the I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. label Jul 20, 2015
@pnkfelix
Copy link
Member

(flagging as I-crash because jemalloc assert fails sound especially scary, in terms of potential threat, to me.)

@alexcrichton
Copy link
Member

Looks like this is indeed a memory unsafety bug. We're allocating a chunk of memory with a size of 16 bytes and then deallocating it with the size of 20 bytes. This was never found because jemalloc assertions are normally disabled, but --enable-debug builds enable these assertions. Optimization and debuginfo are unrelated to this bug.

To see this in action, this program will abort because the assertion in exchange_free is tripped. Note that this doesn't print anything, it just aborts:

#![feature(no_std, lang_items, start, box_syntax, unsize, unique, core_prelude)]
#![feature(core, coerce_unsized, core_intrinsics, libc, linkage)]
#![no_std]
#![allow(improper_ctypes)]

#[macro_use]
extern crate core;
extern crate libc;

use core::prelude::*;
use core::ptr::Unique;
use core::marker::Unsize;
use core::ops::CoerceUnsized;

struct Wrapper<'a, T: ?Sized>(&'a mut i32, T);

impl<'a, T: ?Sized> Drop for Wrapper<'a, T> {
    fn drop(&mut self) {}
}

#[lang = "owned_box"]
struct Box<T>(Unique<T>);
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Box<U>> for Box<T> {}

#[start]
fn start(_argc: isize, _argv: *const *const u8) -> isize {
    let mut x = 3;
    let wrapper: Box<Wrapper<i32>> = box Wrapper(&mut x, 123);
    let _: Box<Wrapper<Send>> = wrapper;
    0
}

#[lang = "exchange_malloc"]
unsafe fn exchange_malloc(size: usize, align: usize) -> *mut u8 {
    assert_eq!(size, 16);
    assert_eq!(align, 8);
    extern {
        fn malloc(size: usize) -> *mut u8;
    }
    malloc(size)
}

#[lang = "exchange_free"]
unsafe fn exchange_free(ptr: *mut u8, old_size: usize, align: usize) {
    assert_eq!(old_size, 16);
    assert_eq!(align, 8);
    extern {
        fn free(ptr: *mut u8);
    }
    free(ptr)
}



#[lang = "stack_exhausted"] extern fn stack_exhausted() {}
#[lang = "eh_personality"] extern fn eh_personality() {}
#[lang = "panic_fmt"]
#[linkage = "external"]
pub fn panic_fmt() -> ! {
    unsafe { core::intrinsics::abort() }
}

Nominating as this seems like a minor codegen bug (perhaps related to the warning that's printed), but I don't think that it's too too urgent to fix immediately.

@alexcrichton alexcrichton changed the title jemalloc assert fail in issue-26709.rs run-pass test Wrong size passed to free in issue-26709.rs run-pass test Jul 20, 2015
@alexcrichton
Copy link
Member

Also updated the title a bit.

@alexcrichton alexcrichton added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2015
@pnkfelix pnkfelix changed the title Wrong size passed to free in issue-26709.rs run-pass test Wrong size passed to free in issue-26709.rs run-pass test (yielding jemalloc assert fail) Jul 21, 2015
@eefriedman
Copy link
Contributor

The testcase can be simplified by just using size_of_val:

struct Wrapper<'a, T: ?Sized>(&'a mut i32, T);
impl<'a, T: ?Sized> Drop for Wrapper<'a, T> {
    fn drop(&mut self) {}
}
fn main() {
    let mut x = 3;
    let wrapper: Box<Wrapper<i32>> = Box::new(Wrapper(&mut x, 123));
    assert_eq!(std::mem::size_of_val(&*wrapper), 16);
    let wrapper2: Box<Wrapper<Send>> = wrapper;
    assert_eq!(std::mem::size_of_val(&*wrapper2), 16); // currently fails
}

This is essentially the same as issue #26403.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 29, 2015
…ion.

This is trickier than it sounds (because the DST code was written
assuming that one could divide the sized and unsized portions of a
type strictly into a sized prefix and unsized suffix, when it reality
it is more like a sized prefix and sized suffix that surround the
single unsized field).

I chose to put in a pretty hack-ish approach to this because
drop-flags are scheduled to go away anyway, so its not really worth
the effort to to make an infrastructure that sounds as general as the
previous paragraph indicates.

Also, I have written notes of other fixes that need to be made to
really finish fixing rust-lang#27023, namely more work needs to be done to
account for alignment when computing the size of a value.
bors added a commit that referenced this issue Aug 4, 2015
…omatsakis

Change the behavior of the glue code emitted for `size_and_align_of_dst`.

This thus changes the behavior of `std::mem::size_of_val` and `std::mem::align_of_val`.  It tries to move us towards a world where the following property holds:

Given type `T` implements `Trait` and a value `b: Box<T>`, where `std::mem::size_of::<T>()` returns `k`, then:

 * `std::mem::size_of_val(b)` returns `k`
 * `std::mem::size_of_val(b as Box<Trait>)` returns `k`

Note that one might legitimately question whether the above property *should* hold.  The property certainly does not hold today, as illustrated by #27023.

(A follow-up task is to make various tests that check that the above property holds for a wide variety of types ... I chose not to invest effort in writing such a test before we actually determine that the above property is desirable.)

nmatsakis and pnkfelix agree that this PR does not require an RFC.  cc @rust-lang/lang (since others may disagree).

(It also *might* break code, though it is hard for me to imagine that it could break code that wasn't already going to assert-fail when run in e.g. debug builds...)

Fix issue #27023

Also, this (or something like it) is a prerequisite for *fixing`make check` on `--enable-optimize --enable-debug` builds*
@alexcrichton
Copy link
Member

I believe this was fixed in #27351

@apasel422
Copy link
Contributor

The following prints 16 followed by 24 on stable, beta, and nightly: https://is.gd/rQdidj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants