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

[C++] Fix wrong Base64 paddings #11578

Conversation

BewareMyPower
Copy link
Contributor

Motivation

This bug was introduced from #11492, which adds padding characters but it will still add 4 =s even if the length can be divided by 4. Following tests are broken currently:

  • ProtobufNativeSchemaTest.testAutoCreateSchema
  • ProtobufNativeSchemaTest.testEndToEnd
  • ProtobufNativeSchemaTest.testSchemaJson
  • ProtobufNativeSchemaTest.testSchemaIncompatibility

Modifications

Only add at most 2 padding characters in Base64 encoding. If the Base64 encoded string's length is 4N+1, there must be something wrong with the encoding, so throw an exception in this case.

Verifying this change

Currently CI of C++ client is broken, we only need to confirm with the workflow output to ensure this fix works.

@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug component/c++ doc-not-needed Your PR changes do not impact docs labels Aug 6, 2021
@BewareMyPower BewareMyPower self-assigned this Aug 6, 2021
@BewareMyPower
Copy link
Contributor Author

Following output is from https://github.com/apache/pulsar/pull/11578/checks?check_run_id=3259005013.

FAILED TESTS (12/248):
      47 ms: ./main AuthPluginTest.testTlsDetectHttps (try #1)
      54 ms: ./main AuthPluginTest.testTlsDetectHttps (try #2)
      31 ms: ./main BasicEndToEndTest.testV2TopicHttp (try #1)
      54 ms: ./main BasicEndToEndTest.testV2TopicHttp (try #2)
      23 ms: ./main BasicEndToEndTest.testHandlerReconnectionLogic (try #1)
      20 ms: ./main BasicEndToEndTest.testHandlerReconnectionLogic (try #2)
      24 ms: ./main AuthPluginToken.testTokenWithHttpUrl (try #1)
      22 ms: ./main AuthPluginToken.testTokenWithHttpUrl (try #2)
    1535 ms: ./main ClientTest.testConnectTimeout (try #1)
    1517 ms: ./main ClientTest.testConnectTimeout (try #2)
      84 ms: ./main ClientDeduplicationTest.testProducerDeduplication (try #1)
      61 ms: ./main ClientDeduplicationTest.testProducerDeduplication (try #2)

We can see ProtobufNativeSchemaTest didn't fail after the change.

@codelipenghui codelipenghui added this to the 2.9.0 milestone Aug 6, 2021
@codelipenghui codelipenghui merged commit 898bb10 into apache:master Aug 6, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-fix-protobuf-schema-padding branch August 6, 2021 08:36
LeBW pushed a commit to LeBW/pulsar that referenced this pull request Aug 9, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants