arrow: [C++][Parquet] Segmentation fault reading modular encrypted Parquet dataset over 2^15 rows

I believe that the reason this issue is only seen with more than 2^15 rows is that this is the value of kMaxBatchSize used in Acero, and when writing the dataset, if there are more rows than this, the Parquet file is split into multiple row groups. As a workaround, I can get the reproduction code from the issue description to work without error if I use ds.write_dataset(table, path, format=file_format, file_options=write_options, min_rows_per_group=row_count), so that Parquet files only ever have one row group.

The reason this leads to crashes when multi-threading is enabled seems to be that when scanning the dataset, RowGroupGenerator::read_one_grow_group is called concurrently from different threads due to using a ReadaheadGenerator, leading to concurrent use of the same AesDecryptor instances, which are not thread safe. Just putting mutexes around use of the AesDecryptors isn’t sufficient to fix the problem due to Decryptor::UpdateAad being used to update the AAD value as data pages are read, which then affects the use of the decryptor from other threads. The InternalFileDecryptor::Get*Decryptor methods are also called concurrently but are not thread safe due to modifying std::maps and the all_decryptors_ vector.

@eirki mentioned seeing this issue without using the Dataset API, and I believe this might also happen when FileReaderImpl::DecodeRowGroups is used to decode multiple row groups concurrently when threading is enabled, but I haven’t tried reproducing that.

Edit: Actually I don’t think this is due to ReadaheadGenerator but rather the readahead logic in RowGroupGenerator. I still get errors when using FragmentReadahead(1) but the errors go away when using BatchReadahead(0). This explains why setting BatchSize to something small fixes the issue, as the default batch readahead is 16 so if 16 * BatchSize is less than the row group size then we won’t try to readahead into another row group. So a better workaround is probably to set batch_readahead=0 in Dataset.to_table from Python.

I have submitted a pull request that addresses this issue: #39623. This PR contains the proposed fix for the segmentation fault encountered in modular encryption as discussed here. @wgtmac

Yes, I can. 👌

I changed GetColumnDecryptor so that it always returns a new Decryptor and I no longer see the issue. I am not sure , without testing, if I am stomping on something else, or if this is efficient since the Decryptor was being cached.

Previous Method:

std::shared_ptr<Decryptor> InternalFileDecryptor::GetColumnDecryptor(
    const std::string& column_path, const std::string& column_key_metadata,
    const std::string& aad, bool metadata) {
  std::string column_key;
  // first look if we already got the decryptor from before
  if (metadata) {
    if (column_metadata_map_.find(column_path) != column_metadata_map_.end()) {
      auto res(column_metadata_map_.at(column_path));
      res->UpdateAad(aad);
      return res;
    }
  } else {
    if (column_data_map_.find(column_path) != column_data_map_.end()) {
      auto res(column_data_map_.at(column_path));
      res->UpdateAad(aad);
      return res;
    }
  }

  column_key = properties_->column_key(column_path);
  // No explicit column key given via API. Retrieve via key metadata.
  if (column_key.empty() && !column_key_metadata.empty() &&
      properties_->key_retriever() != nullptr) {
    try {
      column_key = properties_->key_retriever()->GetKey(column_key_metadata);
    } catch (KeyAccessDeniedException& e) {
      std::stringstream ss;
      ss << "HiddenColumnException, path=" + column_path + " " << e.what() << "\n";
      throw HiddenColumnException(ss.str());
    }
  }
  if (column_key.empty()) {
    throw HiddenColumnException("HiddenColumnException, path=" + column_path);
  }

  // Create both data and metadata decryptors to avoid redundant retrieval of key
  // using the key_retriever.
  int key_len = static_cast<int>(column_key.size());
  auto aes_metadata_decryptor = encryption::AesDecryptor::Make(
      algorithm_, key_len, /*metadata=*/true, &all_decryptors_);
  auto aes_data_decryptor = encryption::AesDecryptor::Make(
      algorithm_, key_len, /*metadata=*/false, &all_decryptors_);

  column_metadata_map_[column_path] = std::make_shared<Decryptor>(
      aes_metadata_decryptor, column_key, file_aad_, aad, pool_);
  column_data_map_[column_path] =
      std::make_shared<Decryptor>(aes_data_decryptor, column_key, file_aad_, aad, pool_);

  if (metadata) return column_metadata_map_[column_path];
  return column_data_map_[column_path];
}

Updated Method

std::shared_ptr<Decryptor> InternalFileDecryptor::GetColumnDecryptor(
    const std::string& column_path, const std::string& column_key_metadata,
    const std::string& aad, bool metadata) {
  std::string column_key = properties_->column_key(column_path);

  column_key = properties_->column_key(column_path);
  // No explicit column key given via API. Retrieve via key metadata.
  if (column_key.empty() && !column_key_metadata.empty() &&
      properties_->key_retriever() != nullptr) {
    try {
      column_key = properties_->key_retriever()->GetKey(column_key_metadata);
    } catch (KeyAccessDeniedException& e) {
      std::stringstream ss;
      ss << "HiddenColumnException, path=" + column_path + " " << e.what() << "\n";
      throw HiddenColumnException(ss.str());
    }
  }
  if (column_key.empty()) {
    throw HiddenColumnException("HiddenColumnException, path=" + column_path);
  }

  int key_len = static_cast<int>(column_key.size());
  auto aes_decryptor =
      encryption::AesDecryptor::Make(algorithm_, key_len, metadata, &all_decryptors_);

  return std::make_shared<Decryptor>(aes_decryptor, column_key, file_aad_, aad, pool_);

}

(also, as far as I understand, while it is definitely a serious bug, it’s already present in the previous major release as well, so it’s not a regression (or at least not a new regression), which I think makes it a little bit less blocking)

Hi @tolleybot , I am currently working on the 15.0.0 release. If someone is able to push a fix before I create a RC in the next days I am happy to cherry-pick the fix but I don’t think we should hold the release, otherwise would be difficult to release due to other bugs/issues. If the issue is solved after the release is done we can also add it to a possible patch release and or to the scheduled quarter release. Of course if there are major concerns and other community members think this should block the release I will act accordingly.

@tmct I recompile the C++ build using ARROW_ENABLE_THREADING = OFF and I have no issue.