gh-146192: Add base32 support to binascii#146193
gh-146192: Add base32 support to binascii#146193kangtastic wants to merge 6 commits intopython:mainfrom
Conversation
Add base32 encoder and decoder functions implemented in C to `binascii` and use them to greatly improve the performance and reduce the memory usage of the existing base32 codec functions in `base64`. No API or documentation changes are necessary with respect to any functions in `base64`, and all existing unit tests for those functions continue to pass without modification. Resolves: pythongh-146192
|
You can now update your PR, @kangtastic. |
|
@serhiy-storchaka Already on it 😄 |
- Use the new `alphabet` parameter in `binascii` - Remove `binascii.a2b_base32hex()` and `binascii.b2a_base32hex()` - Change value for `.. versionadded::` ReST directive in docs for new `binascii` functions to "next" instead of "3.15"
db96a3f to
bf1308f
Compare
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I added some suggestions, but the core LGTM.
Please add assertions for new alphabets in test_constants.
Doc/library/binascii.rst
Outdated
|
|
||
| .. function:: b2a_base32(data, /, *, alphabet=BASE32_ALPHABET) | ||
|
|
||
| Convert binary data to a line(s) of ASCII characters in base32 coding, |
There was a problem hiding this comment.
It is a single line.
I will add wrapcol in a separate issue.
Doc/library/binascii.rst
Outdated
|
|
||
| Convert base32 data back to binary and return the binary data. | ||
|
|
||
| Valid base32 data: |
There was a problem hiding this comment.
This list is incomplete and redundant. I think it is better to follow the example of ascii85 and base85 (with a reference to the RFC). Mention that the mapping is case-sensitive and no optional mapping of the digit "0" and "1" to letters "O", "I" or "l" is used.
Doc/library/binascii.rst
Outdated
|
|
||
| .. data:: BASE32_ALPHABET | ||
|
|
||
| The base32 alphabet according to :rfc:`4648`. |
There was a problem hiding this comment.
| The base32 alphabet according to :rfc:`4648`. | |
| The Base 32 alphabet according to :rfc:`4648`. |
Doc/library/binascii.rst
Outdated
|
|
||
| .. data:: BASE32HEX_ALPHABET | ||
|
|
||
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. |
There was a problem hiding this comment.
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. | |
| The "Extended Hex" Base 32 alphabet according to :rfc:`4648`. |
These are the names used in the table 3 and 4 captions in RFC 4648.
Oh, we can even refer directly to the table:
| The "Extended Hex" base32hex alphabet according to :rfc:`4648`. | |
| The "Extended Hex" Base 32 alphabet according to :rfc:`4648`, table 4. |
Add this also for Base 64 alphabets if you choose this variant.
There was a problem hiding this comment.
I was wondering if this would come up. RFC 4648 uses all four of the terms "Base 32", "Base32", "base 32", and "base32" to refer to this encoding at various points, but it also states e.g.:
This encoding may be referred to as "base32hex". This encoding should not be regarded as the same as the "base32" encoding and should not be referred to as only "base32".
and e.g.:
One property with this alphabet, which the base64 and base32 alphabets lack...
thus implying that "base32" and "base32hex" are preferred, even if the rest of the document doesn't adhere to the implication.
Anyway, I'll refer to it as "Base 32" in docs for now to fit what's already there, and not reference the table number or touch any Base64 stuff so as to keep the scope of this PR limited.
Lib/base64.py
Outdated
| if len(s) % 8: | ||
| raise binascii.Error('Incorrect padding') |
There was a problem hiding this comment.
Should not this be handled in the C code?
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): | ||
| s = _bytes_from_decode_data(s) |
There was a problem hiding this comment.
This is only needed if map01 is not None.
There was a problem hiding this comment.
Correction: it is also needed if casefold is true, for input like 'ß' or 'ffi'.
Lib/base64.py
Outdated
| if alphabet not in _b32rev: | ||
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): |
There was a problem hiding this comment.
I suggest to inline this function. map01 handling is only needed for standard alphabet, and the code for casefold is trivial.
Modules/binascii.c
Outdated
| * | ||
| alphabet: Py_buffer(c_default="{NULL, NULL}") = BASE32_ALPHABET | ||
|
|
||
| base32-code line of data. |
There was a problem hiding this comment.
| base32-code line of data. | |
| Base32-code line of data. |
- Update docs to refer to "Base 32" and "Base32" - Update docs to better explain `binascii.a2b_base32()` - Inline helper function in `base64` - Add forgotten tests for presence of alphabet module globals
|
|
||
| .. data:: BASE32HEX_ALPHABET | ||
|
|
||
| The "Extended Hex" Base 32 alphabet according to :rfc:`4648`. |
There was a problem hiding this comment.
Maybe mention that this one maintains sort order when encoded.
| Optional *alphabet* must be a :class:`bytes` object of length 32 which | ||
| specifies an alternative alphabet. | ||
|
|
||
| Invalid base32 data will raise :exc:`binascii.Error`. |
There was a problem hiding this comment.
How invalid? ex: I think our implementation has always ignored excess bits in the final character that don't map to a byte. We should be explicit about this (and cover the behavior in a test). I think it is reasonable to change our behavior to conform strictly to the "MAY choose to" in https://datatracker.ietf.org/doc/html/rfc4648#section-3.5 and raise the Error when there are excess bits in the input.
We should also have base64's strict_mode=True do the same.
(I do not think we need strict_mode for a2b_base32, we're always being strict here)
There was a problem hiding this comment.
I think this is a separate issue. This PR does not change the behavior of existing decoder.
There are more important issues than checking for canonicity of input. They should be fixed first.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Please add also the What's New entry.
| Optional *alphabet* must be a :class:`bytes` object of length 32 which | ||
| specifies an alternative alphabet. | ||
|
|
||
| Invalid base32 data will raise :exc:`binascii.Error`. |
There was a problem hiding this comment.
I think this is a separate issue. This PR does not change the behavior of existing decoder.
There are more important issues than checking for canonicity of input. They should be fixed first.
Doc/library/binascii.rst
Outdated
| * Contains no excess data after padding (including excess padding, newlines, etc.). | ||
| * Does not start with padding. | ||
| .. note:: | ||
| By default, this function does not map lowercase characters (which are |
There was a problem hiding this comment.
Remove "by default". There are no options for non-default behavior.
There was a problem hiding this comment.
Wouldn't the user specifying an alternative alphabet be a non-default behavior?
There was a problem hiding this comment.
It does not allow to map several characters to the same code.
There was a problem hiding this comment.
It would work for folding 100% lowercase input. Maybe "By itself" instead of "By default"? But I don't mind removing it since it's such a rare hypothetical.
| _b32rev[alphabet] = {v: k for k, v in enumerate(alphabet)} | ||
|
|
||
| def _b32decode_prepare(s, casefold=False, map01=None): | ||
| s = _bytes_from_decode_data(s) |
There was a problem hiding this comment.
Correction: it is also needed if casefold is true, for input like 'ß' or 'ffi'.
- Revise docs - Add whatsnew entry - Minor whitespace change in tests
Referring to a group of 8 bytes as an "octet" may cause confusion, because the term is already commonly used in some languages to refer to a group of 8 bits (i.e. a byte). "Octa" is a suitable preexisting alternative for a group of 64 bits [1] (used by Knuth himself, at that). "Octad" was considered, but it, too, historically refers to a byte. Also rename "quintet" to "quint". "Pentad" was considered, but it historically refers to a group of 5 bits. [1] https://en.wikipedia.org/wiki/Units_of_information
Synopsis
Add base32 encoder and decoder functions implemented in C to
binasciiand use them to greatly improve the performance and reduce the memory usage of the existing base32 codec functions inbase64.No API or documentation changes are necessary with respect to any functions in
base64, and all existing unit tests for those functions continue to pass without modification.Resolves: gh-146192
Discussion
The base32-related functions in
base64are now wrappers for the new functions inbinascii, as envisioned in the docs:Comments and questions are welcome.
Benchmarks
Benchmark script
Unmodified mainline CPython
With this PR
Encoding performance is improved by ~150x, decoding performance is improved by ~200x,
and no auxiliary memory is used.
📚 Documentation preview 📚: https://cpython-previews--146193.org.readthedocs.build/