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

avoid panic in decode_mut #126

Open
dignifiedquire opened this issue Jan 28, 2025 · 4 comments
Open

avoid panic in decode_mut #126

dignifiedquire opened this issue Jan 28, 2025 · 4 comments

Comments

@dignifiedquire
Copy link

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 n0-computer/iroh#3155 for an example of where this was a problem

@ia0
Copy link
Owner

ia0 commented Jan 28, 2025

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.

This was referenced Mar 24, 2024
@ia0
Copy link
Owner

ia0 commented Jan 28, 2025

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.

@dignifiedquire
Copy link
Author

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

@ia0 ia0 added the enhancement label Feb 9, 2025
@ia0
Copy link
Owner

ia0 commented Feb 9, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants