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

API rework #207

Merged
merged 17 commits into from Jan 6, 2023
Merged

API rework #207

merged 17 commits into from Jan 6, 2023

Conversation

marshallpierce
Copy link
Owner

@marshallpierce marshallpierce commented Dec 15, 2022

@marshallpierce marshallpierce force-pushed the mp/api-rework branch 2 times, most recently from cebceb4 to 95c378f Compare December 16, 2022 21:04
@marshallpierce marshallpierce marked this pull request as ready for review December 18, 2022 14:10
Copy link

@djmitche djmitche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updated release notes!

RELEASE-NOTES.md Outdated Show resolved Hide resolved
@Gelbpunkt
Copy link

Hello @marshallpierce,

thanks for this update. I like the concise new API, but there seems to be somewhat of a regression (?) regarding the calculation of output in decode_config_slice.

I've put together an example in this gist. The quick rundown is that I have a websocket implementation that base64-encodes a slice of 20 bytes, which will then be 28 bytes large.

base64 0.13 was able to decode this into a slice with the length 20 just fine, even though it claims to panic if the slice is too small. base64 0.21-beta.2 will error because the length estimate for an input of length 28 returns 21 and it fails early.

I am guessing that this is due to the estimates being conservative, however there is no way for me to decode into a slice of length 20 now, even though it will work! Would it be possible to make the checking of the output length lazy (as I assume that must be how 0.13 did it, I haven't read the source), or adding a method that skips this check and panics instead?

@fundon fundon mentioned this pull request Jan 1, 2023
@marshallpierce
Copy link
Owner Author

  • The *_slice decode/encode functions now return an error instead of panicking when the output slice is too small
    - As part of this, there isn't now a public way to decode into a slice exactly the size needed for inputs that
    aren't multiples of 4 tokens. If adding up to 2 bytes to always be a multiple of 3 bytes for the decode buffer is
    a problem, file an issue.

@Gelbpunkt I see this part has come true. :) I was persuaded in #192 that decode_slice should not panic so that there was a safe way available. However, I don't mind adding back the old behavior, perhaps as decode_slice_unchecked. Would that work for you? It would be more work to make a more precise and/or lazy check, so I can explore that later, but for now I think just adding back the old logic is fine.

@Gelbpunkt
Copy link

Gelbpunkt commented Jan 1, 2023

  • The *_slice decode/encode functions now return an error instead of panicking when the output slice is too small
    • As part of this, there isn't now a public way to decode into a slice exactly the size needed for inputs that
      aren't multiples of 4 tokens. If adding up to 2 bytes to always be a multiple of 3 bytes for the decode buffer is
      a problem, file an issue.

@Gelbpunkt I see this part has come true. :) I was persuaded in #192 that decode_slice should not panic so that there was a safe way available. However, I don't mind adding back the old behavior, perhaps as decode_slice_unchecked. Would that work for you? It would be more work to make a more precise and/or lazy check, so I can explore that later, but for now I think just adding back the old logic is fine.

I agree that not panicking is the better implementation, even for my usecase. It allows for handling malicious user input (or in this case HTTP requests with a header value that'll exceed the buffer). That's why I would prefer lazy checking in the long run, but for now adding back the old behaviour as an unchecked variant would be good enough already. Thank you for the quick response!

@marshallpierce
Copy link
Owner Author

I've captured that in #210. In the meantime, I'll release rc1 with decode_slice_unchecked.

@aolbrech
Copy link

aolbrech commented Jan 4, 2023

Hi,

Just a question on release expectations for "stable" 0.21.0. We ran into problems after upgrading to version 0.20.0, and I have been trying to fix them the last couple of days. Since I noticed work is being done on a version 0.21.0 I used the rc.1 release today and noticed that this again causes some significant API changes.

In case the 0.21.0 release is to be expected in the very near future, it would make more sense to just wait for this release to be made available and change all our code at that point. Just a rough estimate would be helpful.

Thanks!

@marshallpierce
Copy link
Owner Author

If people don't use the release candidates, there's no point to publishing them -- they're there to gather feedback. :)

Since there's been no feedback thus far on rc.1, I'll probably release it as 0.21.0 in the next few days.

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

Successfully merging this pull request may close these issues.

None yet

4 participants