lib: remove the comment of base64 validation #29201
Conversation
There was a problem hiding this comment.
Using a regex slows the cipher/hash update()s considerably (at least by half for a 1024 character string in my quick benchmarking). I think simply adding a length check would suffice.
If we wanted to be extra strict, we could check the number of bytes written in the C++ layer or something to help determine how valid the base64 string was, provided it didn't cause a serious performance regression.
addaleax
added
semver-major
|
I’m sorry but I’m -1, this is a major breaking change that has the potential to break a lot of code with little benefit, we just shouldn’t do it at all imo. |
|
Then we should just remove the comment or replace it with a comment explaining why we're not doing validation? FWIW, I agree with Anna's assessment. |
SEWeiTung
changed the title
Since there's a comment about the 'base64' validation needed, and this will result in a breaking change, so this should be removed.
|
@addaleax，OK, I've removed this comment to keep it clear. |
mscdex
removed
the
semver-major
|
@mscdex Are you OK with removing the comment? @MaledongGit It may be better/easier to close this and open this simple change as a new PR. /ping @bnoordhuis |
Trott
added
the
author ready
Since there's a comment about the 'base64' validation needed, and this will result in a breaking change, so this should be removed. PR-URL: #29201 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
|
Landed in d5c3837 🎉 🎉 🎉 🎉 |
Since there's a comment about the 'base64' validation needed, and this will result in a breaking change, so this should be removed.
make -j4 test(UNIX), orvcbuild test(Windows) passes