force casting Vec<T> to &[T] in Table::AsRef is causing trouble

[david0u0]

The compiler version: rustc 1.67.0-nightly (c090c6880 2022-12-01)

As shown in the image in gdb:

• $8 is a ref to Table
• $7 is a ref to TableSlice, returned by Table::AsRef

They have identical pointer value, 0x7ffffff782e0, but the rows field in them are in different address. rows of Table is the correct one, while rows of TableSlice contains random data.

Further more, the rows of TableSlice has length = 93825009397280 (!!!).

The cause is in the implementation of Table::AsRef. It force casts a &Table into &TableSlice, and when the layout of these two types are not the same, it'll cause trouble.

impl<'a> AsRef<TableSlice<'a>> for Table {
    fn as_ref(&self) -> &TableSlice<'a> {
        unsafe {
            // All this is a bit hacky. Let's try to find something else
            let s = &mut *((self as *const Table) as *mut Table);
            s.rows.shrink_to_fit();
            &*(self as *const Table as *const TableSlice<'a>)
        }
    }
}

[david0u0]

More info:

1. It was fine with rustc 1.60.0-nightly (88fb06a1f 2022-02-05). Only when I migrate to latest rustc did I see the bug.
2. I added #[repr(C)] to both structures: now the rows field have the same address, but the length of TableSlice.rows is still corrupted.

[cschwan]

I have the same problem, see here: NNPDF/pineappl#195.

[david0u0]

Also, the signature impl<'a> AsRef<TableSlice<'a>> for Table can lead to undefined behavior: what if 'a is 'static, and the table considered here is a local var?

So there's no way to fix it without breaking change, unless you leak memory indefinitely

consider following code:

    #[test]
    fn doom() {
        let s: super::TableSlice;
        {
            let t = Table::new();
            s = t.as_ref().clone();
        }
        println!("{:?}", s);
    }

This 100% safe rust will lead to segfault

[cschwan]

@david0u0 Would replacing &self with &'a self fix this? This is probably what the author meant.

[david0u0]

@cschwan I think it won't compile, it will contradict the definition of AsRef

[andrewpollack]

+1 to this issue -- the following minimal test is leading to segfaults on nightly (and confirmed fixed by #146)

let _ = cell!(table!(
    ["a", "b"],
    ["c", "d"]
));

[obsgolem]

That entire function is a rats nest of undefined behavior. First, casting a reference to a mut reference is undefined behavior, you can easily get into a situation where you have both a &Table and and an &mut Table outstanding simultaneously, instantly rendering the program undefined. Additionally neither Table nor TableSlice are repr(C), and hence it is undefined behavior to try and access one via a pointer or reference to the other. The compiler may reorder structs arbitrarily, as seems to have happened here. A Vec is not guaranteed to have an initial sequence identical to a slice...

You can easily fix this by changing to something like

fn as_ref(&self) -> &TableSlice<'a> {
    TableSlice {
        format: &*self.format,
        titles: &*self.titles,
        rows: self.rows.as_slice()
    }
}

[pinkforest]

Hey lovely Op, could you please do a informational = "unsound" PR at https://github.com/rustsec/advisory-db ?

I'll merge a fix for 1.0.0 - need to bring other stuff up-to-date as well.

Mark as fixed in 1.0.0 - pondering whether i should bother with a pre-release given a breaking change

Closing this when the release is out in crates.io and advisory is out.

[david0u0]

@pinkforest now that the PR is merged, is it still required to mark it as unsound?

[pinkforest]

Yes. All the older version need to be marked as such in crates.io and get people moving to 1.0.0. I have some work to do to get to is-terminal and termcolor and do some lotto for MSRV

[david0u0]

I create a PR at rustsec/advisory-db#1503

[pinkforest]

0.10.0 Released and advisory is out 🥳

(I thought 1.0.0 was too premature)