Skip to content

Introduce unified checksum options and deprecate old options#2

Open
v-pratap wants to merge 9 commits into
mainfrom
unified-checksum-options-pr1
Open

Introduce unified checksum options and deprecate old options#2
v-pratap wants to merge 9 commits into
mainfrom
unified-checksum-options-pr1

Conversation

@v-pratap

Copy link
Copy Markdown
Owner

Introduces unified enum-based UploadChecksumValidationOption and DownloadChecksumValidationOption and deprecates the old bool-based EnableMD5ValidationOption and EnableCrc32cValidationOption.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces new options for configuring upload and download checksum validation (UploadChecksumValidationOption and DownloadChecksumValidationOption) while deprecating older hashing options. It also updates the asynchronous writer connection to support manual checksum verification during finalization and adds corresponding tests and examples. The review feedback highlights two critical issues: first, the unsafe use of OptionsSpan across a co_await suspension point in the new sample code, which can leak thread-local state; second, a bug in AsyncWriterConnectionImpl::Finalize where current_options is checked instead of merged for non-appendable uploads, causing initial upload options to be ignored.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +751 to +757
// Set the expected CRC32C checksum in the current options scope
// just before calling Finalize().
// Note: 548262564U is the pre-computed CRC32C checksum for the string "Some
// data\n". If the data changes, this checksum must be updated to match.
google::cloud::internal::OptionsSpan span(
google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(548262564U));
co_return (co_await writer.Finalize(std::move(token))).value();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using google::cloud::internal::OptionsSpan across a co_await point is highly dangerous. OptionsSpan is an RAII object that manages thread-local state. Because a coroutine can suspend at co_await and resume on a different thread (or allow other tasks to run on the same thread in the meantime), keeping OptionsSpan alive across co_await can leak the options context to unrelated tasks or fail to restore the thread-local state correctly on resumption.

To fix this, invoke writer.Finalize synchronously inside a lambda or block where the OptionsSpan is created and destroyed before the co_await suspension point.

    // Set the expected CRC32C checksum in the current options scope
    // just before calling Finalize().
    // Note: 548262564U is the pre-computed CRC32C checksum for the string "Some
    // data\n". If the data changes, this checksum must be updated to match.
    auto finalize_future = [&] {
      google::cloud::internal::OptionsSpan span(
          google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(548262564U));
      return writer.Finalize(std::move(token));
    }();
    co_return (co_await std::move(finalize_future)).value();

Comment on lines +163 to +179
} else if (current_options
.has<google::cloud::storage::UseCrc32cValueOption>() ||
current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
// If the user specified a manual expected checksum dynamically at
// Finalize() via current_options, we manually inject it and use `kFinalize`
// so the internal hash function doesn't overwrite it with its own computed
// hash.
if (current_options.has<google::cloud::storage::UseCrc32cValueOption>()) {
write.mutable_object_checksums()->set_crc32c(
current_options.get<google::cloud::storage::UseCrc32cValueOption>());
}
if (current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
write.mutable_object_checksums()->set_md5_hash(
current_options.get<google::cloud::storage::UseMD5ValueOption>());
}
action = PartialUpload::kFinalize;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In the else if block for non-appendable uploads, the code checks current_options instead of merged. This means that if the user specified UseCrc32cValueOption or UseMD5ValueOption at the start of the upload (which is stored in options_ and merged into merged), those options will be completely ignored during Finalize().

To ensure options specified at both the start of the upload and dynamically at Finalize() are respected, use merged instead of current_options in this block, consistent with how appendable uploads are handled.

  } else if (merged.has<google::cloud::storage::UseCrc32cValueOption>() ||
             merged.has<google::cloud::storage::UseMD5ValueOption>()) {
    // If the user specified a manual expected checksum, we manually inject it
    // and use `kFinalize` so the internal hash function doesn't overwrite it
    // with its own computed hash.
    if (merged.has<google::cloud::storage::UseCrc32cValueOption>()) {
      write.mutable_object_checksums()->set_crc32c(
          merged.get<google::cloud::storage::UseCrc32cValueOption>());
    }
    if (merged.has<google::cloud::storage::UseMD5ValueOption>()) {
      write.mutable_object_checksums()->set_md5_hash(
          merged.get<google::cloud::storage::UseMD5ValueOption>());
    }
    action = PartialUpload::kFinalize;
  }

@v-pratap
v-pratap force-pushed the unified-checksum-options-pr1 branch from 1264fa0 to 55ceb4c Compare July 15, 2026 09:28
@v-pratap
v-pratap force-pushed the unified-checksum-options-pr1 branch from 55ceb4c to 6580fd9 Compare July 15, 2026 10:15
@v-pratap
v-pratap force-pushed the unified-checksum-options-pr1 branch from 6580fd9 to 6c92722 Compare July 15, 2026 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants