Skip to content

[5.0] Fix base64 encoding - take 2#1888

Merged
heifner merged 4 commits intorelease/5.0from
GH-1461-base64-pad-5.0
Nov 13, 2023
Merged

[5.0] Fix base64 encoding - take 2#1888
heifner merged 4 commits intorelease/5.0from
GH-1461-base64-pad-5.0

Conversation

@heifner
Copy link
Contributor

@heifner heifner commented Nov 13, 2023

#1482 fixed base64 encoding by removing the unneeded extra = that variant added to base64 encoded data. However, it also updated the base64 library with a stricter one. This PR leaves the old base64 encoding/decoding library but does remove the extra = from the base64 encoded data so non-fc base64 decoders will not claim the data is invalid.

This change still breaks backward compatibility with 3.2 & 4.0 cleos/nodeos. PRs to 3.2 (#1889) and (#1892) 4.0 will be created that will allow them to work with this new base64 encoding (they will no longer expect the invalid = character).

PR #1886 reverts #1482. This PR provided an alternative fix for #1461.

Included in this PR is an optimization to the existing base64_decode to avoid a copy for our use-cases by returning a vector<char> instead of a string.
Included in this PR is a change to the nodeos_run_test.py to keep the keosd running with passed --leaving-running which is useful for local testing. This was used to manually test using 3.2 cleos with 5.0.

Resolves #1461

@heifner heifner added the OCI Work exclusive to OCI team label Nov 13, 2023
@spoonincode
Copy link
Contributor

Included in this PR is an optimization to the existing base64_decode to avoid a copy for our use-cases by returning a vector<char> instead of a string

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

@heifner
Copy link
Contributor Author

heifner commented Nov 13, 2023

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

Good point. I'm not a fan of std::string.data() required to be null terminated. In my mind that is what c_str() is for. Not sure why they made that change. I don't buy the removal of COW possibility a valid argument to make data() and c_str() the same thing. Oh well. I didn't find any uses of base64_decode(.*).data() or returns to auto.

@greg7mdp
Copy link
Contributor

I'm not a fan of std::string.data() required to be null terminated. In my mind that is what c_str() is for

The issue is that if std::string.data() was not null terminated, c_str() would have to allocate a new string to add the \0 at the end, and then when would that string be freed?

@greg7mdp
Copy link
Contributor

This has the potential to affect downstream usages when base64_decode("").data() since the return value will be different due to this modification. I didn't spot any problematic usages but it's worth taking a moment to think on it.

For better compatibility, maybe we should add a '\0' at the end of the vector (not included in its size()). This would be lost if the vector is copied, but would address the use case you mentioned above.

@heifner
Copy link
Contributor Author

heifner commented Nov 13, 2023

For better compatibility, maybe we should add a '\0' at the end of the vector (not included in its size()). This would be lost if the vector is copied, but would address the use case you mentioned above.

You mean append a \0 and then resize by size()-1. Not a fan of that.

// fc version of base64_decode allows for extra `=` at the end of the string.
// Other base64 decoders will not accept the extra `=`.
std::vector<char> b64 = base64_decode( str );
return blob( { std::move(b64) } );
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor detail

Suggested change
return blob( { std::move(b64) } );
return { std::move(b64) };

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

Labels

OCI Work exclusive to OCI team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants