data-encoding icon indicating copy to clipboard operation
data-encoding copied to clipboard

avoid panic in `decode_mut`

Open dignifiedquire opened this issue 11 months ago • 4 comments

It seems like a footgun to use an assert in decode_mut for checking the length. It would be much nicer if it returned an error instead, if the length do not much.

See https://github.com/n0-computer/iroh/pull/3155 for an example of where this was a problem

dignifiedquire avatar Jan 28 '25 11:01 dignifiedquire

Thanks for the feature request. Panics should only be used for unrecoverable errors. That's a very good point and hopefully something that can be fixed without a breaking change. I'll try to fix that and other similar issues (if any) this weekend.

Unrelated, but since you pointed me to your code, you may be interested to use data-encoding-macro to save the uppercase translation cost:

static BASE32_NOPAD_PERMISSIVE: data_encoding::Encoding = data_encoding_macro::new_encoding! {
    symbols: "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567",
    translate_from: "abcdefghijklmnopqrstuvwxyz",
    translate_to: "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
};

That said, I should actually add this constant to data-encoding. Both BASE32_DNSSEC and BASE32_DNSCURVE already do it. I'll keep this bug open to fix both issues.

ia0 avatar Jan 28 '25 16:01 ia0

Sadly, it would be a breaking change so I've added the feature request to #72 and is tracked in #106 for v3. If this is still an issue for you, I could provide a new function (e.g. decode_mut_nopanic) since that would be a minor change only.

For the additional permissive base32, I'm adding a case-insensitive one for your use-case and a visual-approximating one since base32 is meant to be handled by humans. This is done in #127. I'll let it open for a week or so to be sure I'm not missing something. After which it will probably be released one more week later or so.

ia0 avatar Jan 28 '25 20:01 ia0

Sadly, it would be a breaking change so I've added the feature request to #72 and is tracked in #106 for v3. If this is still an issue for you, I could provide a new function (e.g. decode_mut_nopanic) since that would be a minor change only.

thanks, it's not super urgent to fix, as i am working around it for now

For the additional permissive base32, I'm adding a case-insensitive one for your use-case and a visual-approximating one since base32 is meant to be handled by humans. This is done in #127. I'll let it open for a week or so to be sure I'm not missing something. After which it will probably be released one more week later or so.

thanks, will take a look

dignifiedquire avatar Jan 28 '25 21:01 dignifiedquire

I've released 2.8.0 which provides BASE32_NOPAD_NOCASE, which saves the need for converting to uppercase first.

I'm keeping this issue open for the initial concern: returning an error instead of panic in decode_mut. This will be done in v3 only.

ia0 avatar Feb 09 '25 11:02 ia0