GH-49614: [C++] Report an error instead of silent truncation in base64_decode on invalid input

[Reranko05]

Rationale for this change

arrow::util::base64_decode previously allowed invalid input to be processed, which could result in silently truncated or incorrect output without signaling an error. This can lead to unintended data corruption.

What changes are included in this PR?

• Change base64_decode to return arrow::Result<std::string> instead of std::string
• Add validation for:
  • invalid input length
  • invalid base64 characters
  • incorrect padding
• Return an error (Status::Invalid) for invalid input instead of producing partial output
• Update all call sites to handle Result<std::string>
• Add unit tests covering valid and invalid inputs

Are these changes tested?

Yes. Unit tests have been added to verify:

• valid decoding behavior
• invalid input length
• invalid characters
• incorrect padding handling

Are there any user-facing changes?

• The API now returns arrow::Result<std::string> instead of std::string
• Invalid base64 input now results in an error (Status::Invalid) instead of returning partial or incorrect output

• GitHub Issue: [C++] arrow::util::base64_decode silently truncates on invalid characters instead of reporting an error #49614

[github-actions]

⚠️ GitHub issue #49614has been automatically assigned in GitHub to PR creator.

[Reranko05]

Thanks for the feedback. I’ve updated the implementation and tests.

• Added stricter validation (length, padding placement/count, allowed characters)
• Removed early termination in the decode loop to avoid silent truncation
• Expanded test coverage to include invalid inputs and edge cases

All tests pass locally. Please let me know if any further adjustments are needed.

[kou]

Could you use arrow::Result<std::string> return type instead of using ARROW_LOG()?

[kou]

FYI: You can run CI on your fork by enabling GitHub Actions on your fork.

[Reranko05]

Could you use arrow::Result<std::string> return type instead of using ARROW_LOG()?

Thanks for the suggestion @kou !

Just to clarify, would you prefer changing the existing base64_decode API to return arrow::Resultstd::string, or introducing a separate checked variant while keeping the current API unchanged?

I want to make sure the approach aligns with existing usage and expectations.

[kou]

"changing the existing base64_decode API to return arrow::Resultstd::string".
But I want to know how many changes are required for existing code that use base64_decode().

[Reranko05]

@kou I checked the current usages of base64_decode(), and it appears to be used in a very limited number of places (primarily in tests and one internal call site in flight_test.cc).

Updating to arrow::Result<std::string> would require adjusting those call sites to use ARROW_ASSIGN_OR_RAISE, but the impact seems quite localized and manageable.

I can proceed with the API change and update the affected call sites accordingly.

[Reranko05]

Hi @kou, just following up on this.

I can proceed with updating base64_decode() to return arrow::Result<std::string> and adjust the affected call sites accordingly. Please let me know if this approach looks good, or if you'd prefer any alternative.

Happy to proceed based on your guidance.

[kou]

Oh, sorry. I forgot to reply this...

Yes. Let's proceed with arrow::Result<std::string>.

[Reranko05]

Hi @kou, thanks for confirming!

I’ve updated base64_decode() to return arrow::Result<std::string> and added validation for invalid inputs (length, padding, and non-base64 characters). I also updated the tests accordingly.

All tests are passing locally. Please let me know if you’d like any changes or adjustments.

[Copilot]

Pull request overview

This PR addresses a correctness issue in Arrow’s C++ base64 decoder by ensuring malformed base64 input is detected instead of producing silently truncated/partial output.

Changes:

• Adds pre-validation for base64 input (length, padding placement, invalid characters) in base64_decode.
• Changes base64_decode API to return arrow::Result<std::string> with Status::Invalid on malformed input.
• Adds unit tests covering valid/invalid decoding cases (and adjusts a couple of ToChars assertions).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
cpp/src/arrow/vendored/base64.cpp	Adds base64 input validation and switches decode to return errors instead of partial output.
cpp/src/arrow/util/base64.h	Updates public API signature of base64_decode to return Result<std::string>.
cpp/src/arrow/util/string_test.cc	Adds tests for base64 decode validity/error cases and tweaks ToChars expectations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Reranko05]

Hi @kou, I have addressed all review comments:

• Removed redundant helpers and reused existing base64_chars
• Merged validation into decoding loop (single-pass)
• Fixed padding validation to ensure '=' only appears at the end
• Cleaned up includes and namespace usage
• Updated tests to improve coverage and follow Arrow conventions

All tests pass locally.

[kou]

Could you enable GitHub Actions on your fork to run CI on your fork too?

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Reranko05]

Hi @kou, I have addressed the remaining feedback by using PARQUET_ASSIGN_OR_THROW and updating error handling to return deterministic, case-specific messages. All CI checks are passing.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kou]

+1

@dmitry-chirkov-dremio Do you want to review this before we merge this?

[dmitry-chirkov-dremio]

@dmitry-chirkov-dremio Do you want to review this before we merge this?

Yes, give me 24h.

[dmitry-chirkov-dremio]

Looks good to me however won't approve until conversations are resolved (including Copilot's comments). Leaving couple of comments of my own as well.

Good job, @Reranko05 - almost there.
cc @kou

[Reranko05]

Good job, @Reranko05 - almost there. cc @kou

Thank you @dmitry-chirkov-dremio, I've addressed all review comments and resolved outstanding threads.

[dmitry-chirkov-dremio]

LGTM

[kou]

Thanks. I'll merge this.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d9aeaa1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d9aeaa1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.