force casting Vec<T> to &[T] in Table::AsRef is causing trouble about prettytable-rs HOT 12 CLOSED

0.10.0 Released and advisory is out 🥳

(I thought 1.0.0 was too premature)

from prettytable-rs.

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

from prettytable-rs.

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.

from prettytable-rs.

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

from prettytable-rs.

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

from prettytable-rs.

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

from prettytable-rs.

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

from prettytable-rs.

+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"]
));

from prettytable-rs.

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()
    }
}

from prettytable-rs.

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.

from prettytable-rs.

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

from prettytable-rs.

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

from prettytable-rs.