Switch to our own Base64 implementation by davidebeatrici · Pull Request #13 · mumble-voip/libmumble

davidebeatrici · 2023-03-15T07:43:26Z

Written in modern C++, it strictly adheres to the Base64 specification.

This means that alternate alphabets and/or the lack of padding are rejected.

A benchmark would be nice to have, once we switch to a proper test framework.

Krzmbrzl · 2023-03-15T08:04:39Z

A benchmark would be nice to have, once we switch to a proper test framework.

-> https://github.com/google/benchmark
I have used that one before and it is pretty amazing.

include/mumble/Base64.hpp

davidebeatrici · 2023-03-15T22:01:40Z

Windows build failure

Error: D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): error C2672: 'std::min': no matching overloaded function found
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(74): note: could be '_Ty std::min(std::initializer_list<_Elem>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: '_Ty std::min(std::initializer_list<_Elem>)': expects 1 arguments - 2 provided
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(71): note: or       '_Ty std::min(std::initializer_list<_Elem>,_Pr)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: '_Ty std::min(std::initializer_list<_Elem>,_Pr)': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unsigned long'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(64): note: or       'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)': template parameter '_Ty' is ambiguous
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: could be 'gsl::span<uint8_t,18446744073709551615>::size_type'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: or       'unsigned long'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)': could not deduce template argument for 'const _Ty &' from 'gsl::span<uint8_t,18446744073709551615>::size_type'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(54): note: or       'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept(<expr>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(73): note: 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept(<expr>)': expects 3 arguments - 2 provided
Error: D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): error C2672: 'std::min': no matching overloaded function found
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(74): note: could be '_Ty std::min(std::initializer_list<_Elem>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: '_Ty std::min(std::initializer_list<_Elem>)': expects 1 arguments - 2 provided
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(71): note: or       '_Ty std::min(std::initializer_list<_Elem>,_Pr)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: '_Ty std::min(std::initializer_list<_Elem>,_Pr)': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unsigned long'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(64): note: or       'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)': template parameter '_Ty' is ambiguous
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: could be 'gsl::span<const uint8_t,18446744073709551615>::size_type'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: or       'unsigned long'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &) noexcept(<expr>)': could not deduce template argument for 'const _Ty &' from 'gsl::span<const uint8_t,18446744073709551615>::size_type'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\utility(54): note: or       'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept(<expr>)'
D:\a\libmumble\libmumble\include\mumble/Base64.hpp(122): note: 'const _Ty &std::min(const _Ty &,const _Ty &,_Pr) noexcept(<expr>)': expects 3 arguments - 2 provided

This is of course due to size_t being unsigned long long instead of unsigned long.

Unfortunately the proper literal was only introduced in C++23: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0330r8.html

We can get away with size_t{ value } though.

Krzmbrzl

Essentially all comments on the decode function also apply to the encode one.

include/mumble/Base64.hpp

Krzmbrzl · 2023-03-16T18:05:47Z

Oh and another thing to consider: As you want to implement this header-only anyway, we could also template the functions to allow any type of the correct size (or anything that has at least the appropriate size). That way, we wouldn't have the uint8_t vs std::byte vs char vs ... problem

davidebeatrici · 2023-03-17T08:26:44Z

The failing Windows builds are probably the due to a missing <stdexcept> include.

Oh and another thing to consider: As you want to implement this header-only anyway, we could also template the functions to allow any type of the correct size (or anything that has at least the appropriate size). That way, we wouldn't have the uint8_t vs std::byte vs char vs ... problem

Or rather, template wrappers for them: thanks to the spans we can get away with simple casts.

Krzmbrzl · 2023-03-17T09:20:31Z

thanks to the spans we can get away with simple casts.

Doesn't seem like it: https://godbolt.org/z/5P5dWc5nj 👀

davidebeatrici · 2023-03-17T21:41:53Z

Sorry, by "simple" I meant casting the element type and specifying the size accordingly.

Krzmbrzl · 2023-03-18T09:13:08Z

Maybe we should define our own span_cast that does this under the hood. That will probably make our code a bit cleaner

davidebeatrici · 2023-03-18T10:22:20Z

Indeed, I was actually thinking about it. There are quite a few places in our codebase where it would be useful.

include/mumble/Base64.hpp

Krzmbrzl

Only some small stuff is left. Once this has been addressed, I think this implementation is ready to go 👍

include/mumble/Base64.hpp

Krzmbrzl · 2023-03-30T16:21:03Z

include/mumble/Base64.hpp

+		for (; !in.empty(); out = out.subspan(3), in = in.subspan(4)) {
+			out[0] = (charToByte(in[0]) << std::byte(2)) + ((charToByte(in[1]) & std::byte(0x30)) >> 4);
+
+			if (in[2] == '=') {
+				break;
+			}
+
+			out[1] =
+				((charToByte(in[1]) & std::byte(0x0f)) << std::byte(4)) + ((charToByte(in[2]) & std::byte(0x3c)) >> 2);
+
+			if (in[3] == '=') {
+				break;
+			}
+
+			out[2] = ((charToByte(in[2]) & std::byte(0x03)) << std::byte(6)) + charToByte(in[3]);
+		}


I think there should be a comment that textually describes how the decoding works

include/mumble/Base64.hpp

Krzmbrzl · 2023-03-30T16:24:12Z

include/mumble/Base64.hpp

+		for (; !in.empty(); out = out.subspan(4)) {
+			out[0] = byteToChar((in[0] & std::byte(0xfc)) >> 2);

-	Base64();
-	virtual ~Base64();
+			if (in.size() >= 2) {
+				out[1] = byteToChar(((in[0] & std::byte(0x03)) << 4) + ((in[1] & std::byte(0xf0)) >> 4));

-	virtual explicit operator bool();
+				if (in.size() >= 3) {
+					out[2] = byteToChar(((in[1] & std::byte(0x0f)) << 2) + ((in[2] & std::byte(0xc0)) >> 6));
+					out[3] = byteToChar(in[2] & std::byte(0x3f));
+				} else {
+					out[2] = byteToChar((in[1] & std::byte(0x0f)) << 2);
+					out[3] = PAD_CHAR;
+				}
+			} else {
+				out[1] = byteToChar((in[0] & std::byte(0x03)) << 4);
+				out[2] = PAD_CHAR;
+				out[3] = PAD_CHAR;
+			}

-	virtual size_t decode(const BufView out, const BufViewConst in);
-	static size_t encode(const BufView out, const BufViewConst in);
+			// All data blocks are guaranteed to be 3 bytes except the last one.
+			in = in.subspan(std::min(size_t{ 3 }, in.size()));
+		}


Analogous to the decode function, a comment textually explaining how this stuff works would be good.

Krzmbrzl

Oh and of course: All functions require documentation before merging

I would suggest JavaDoc-style Doxygen comments as those are much easier to distinguish from regular comments than blocks of /// comments. E.g. like this

/**
 * Description of function goes here
 * 
 * @param in The buffer to decode
 */

include/mumble/Base64.hpp

Written in modern C++, it strictly adheres to the Base64 specification. This means that the lack of padding is rejected when decoding. The alternate alphabet that is used for URLs is handled just fine. A benchmark would be nice to have, once we switch to a proper test framework.

Krzmbrzl · 2023-04-03T16:34:22Z

Just to be explicit: The only thing missing from my POV is the documentation (+ the mentioned in-source explanatory comments)

davidebeatrici requested a review from Krzmbrzl March 15, 2023 07:43

Krzmbrzl reviewed Mar 15, 2023

View reviewed changes

include/mumble/Base64.hpp Outdated Show resolved Hide resolved

Krzmbrzl reviewed Mar 15, 2023

View reviewed changes

include/mumble/Base64.hpp Outdated Show resolved Hide resolved

davidebeatrici force-pushed the base64-new-implementation branch from ef07c28 to 12b830c Compare March 15, 2023 22:05

Krzmbrzl requested changes Mar 16, 2023

View reviewed changes

davidebeatrici force-pushed the base64-new-implementation branch from 12b830c to 500d815 Compare March 17, 2023 08:16

davidebeatrici force-pushed the base64-new-implementation branch from 500d815 to cf40972 Compare March 18, 2023 19:59

davidebeatrici requested a review from Krzmbrzl March 21, 2023 07:45

Krzmbrzl reviewed Mar 21, 2023

View reviewed changes

davidebeatrici force-pushed the base64-new-implementation branch from cf40972 to cceeee2 Compare March 24, 2023 05:26

davidebeatrici requested a review from Krzmbrzl March 24, 2023 05:29

davidebeatrici force-pushed the base64-new-implementation branch from cceeee2 to 3828dc9 Compare March 30, 2023 00:50

Krzmbrzl reviewed Mar 30, 2023

View reviewed changes

include/mumble/Base64.hpp Outdated Show resolved Hide resolved

davidebeatrici force-pushed the base64-new-implementation branch from 3828dc9 to 00f3898 Compare March 30, 2023 22:42

Uh oh!

Conversation

davidebeatrici commented Mar 15, 2023

Uh oh!

Krzmbrzl commented Mar 15, 2023

Uh oh!

Uh oh!

Uh oh!

davidebeatrici commented Mar 15, 2023

Uh oh!

Krzmbrzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Krzmbrzl commented Mar 16, 2023

Uh oh!

davidebeatrici commented Mar 17, 2023

Uh oh!

Krzmbrzl commented Mar 17, 2023

Uh oh!

davidebeatrici commented Mar 17, 2023

Uh oh!

Krzmbrzl commented Mar 18, 2023

Uh oh!

davidebeatrici commented Mar 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Krzmbrzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Krzmbrzl Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Krzmbrzl Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Krzmbrzl commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants