Internal symbols array is publicly exposed and unsound

[Manishearth]

use data_encoding::Specification;

fn main() {
    let mut hex = {
        let mut spec = Specification::new();
        spec.symbols.push_str("0123456789abcdef");
        spec.encoding().unwrap()
    };


    hex.0 = (&[0xF6; 514][..]).into(); // created with non-ascii symbols
    let invalid_string = hex.encode(&[1, 2, 3, 4]);
   
    println!("created invalid string");
    println!("{:?}", invalid_string); // will panic
}

This does involve tweaking the field of the Encoding, but that's a public and not doc(hidden) field. At the very least it should be doc(hidden).

(at a meta level, this code would probably benefit from some kind of internal #[repr(transparent)] pub Ascii(u8); type so that it is very clear that the invariants are being upheld)

[ia0]

Thanks for the bug report! I didn't know it was possible to have doc(hidden) on fields. The field needs to be public for data-encoding-macro. The type was already doc(hidden) and is named InternalEncoding which are good indicators that users should not touch it. But making the field itself doc(hidden) is also another good hint. Sending #76 to fix that.

Note that eventually, when const fn support is powerful enough, data-encoding-macro would disappear and the field could become private again. Actually, it's probably also a good idea to make the field private and only expose an unsafe const fn, but that would be a breaking change because data-encoding-macro does not depend on an exact version. Adding to the wish list in #72.

[Manishearth]

@ia0 it may also be worth renaming the field something suitably scary, because it is relatively easy to not notice that x.0 is forbidden: it shows up in error messages and stuff.

I'd also recommend against using doc(hidden) fields when there are unsafe invariants; if possible it would be better to expose a const unsafe fn constructor and call it from the macro.

[ia0]

Yes, I think the const unsafe fn is the best option. Actually, I misremembered, I already have and use a const fn so I can make the field private (that might have been an oversight, or maybe deliberate to hide a breaking change over a few years). Making the const fn unsafe would still be a breaking change though, but I could also hide it under a few years (i.e. assuming users don't use a data-encoding version older by a year than the data-encoding-macro version).

[Manishearth]

Yeah I think a doc(hidden) const fn is better than having a public-but-hidden field, you have to very explicitly find and call that API.