Repository: apache/arrow
File: cpp/src/parquet/decoder.cc (C++)

Code before:
  }

 private:
  Status DecodeArrowDense(int num_values, int null_count, const uint8_t* valid_bits,
                          int64_t valid_bits_offset,
                          typename EncodingTraits<ByteArrayType>::Accumulator* out,
                          int* out_values_decoded) {
    // We're going to decode `num_values - null_count` PLAIN values,
    // and each value has a 4-byte length header that doesn't count for the
    // Arrow binary data length.
    int64_t estimated_data_length = len_ - 4 * (num_values - null_count);
    if (ARROW_PREDICT_FALSE(estimated_data_length < 0)) {
      return Status::Invalid("Invalid or truncated PLAIN-encoded BYTE_ARRAY data");
    }

    auto visit_binary_helper = [&](auto* helper) {
      int values_decoded = 0;

      auto visit_run = [&](int64_t position, int64_t run_length, bool is_valid) {
        if (is_valid) {
          for (int64_t i = 0; i < run_length; ++i) {
            // We ensure `len_` is sufficient thanks to:
            //   1. the initial `estimated_data_length` check above,
            //   2. the running `value_len > estimated_data_length` check below.
            // This precondition follows from those two checks.
            DCHECK_GE(len_, 4);
            auto value_len = SafeLoadAs<int32_t>(data_);
            if (ARROW_PREDICT_FALSE(value_len < 0 || value_len > estimated_data_length)) {
              return Status::Invalid(
                  "Invalid or truncated PLAIN-encoded BYTE_ARRAY data");
            }
            RETURN_NOT_OK(
                helper->AppendValue(data_ + 4, value_len, estimated_data_length));
            auto increment = value_len + 4;
            data_ += increment;
            len_ -= increment;
            estimated_data_length -= value_len;
            DCHECK_GE(estimated_data_length, 0);
          }
          values_decoded += static_cast<int>(run_length);
          return Status::OK();
        } else {
          return helper->AppendNulls(run_length);
        }
      };

      RETURN_NOT_OK(
          VisitBitRuns(valid_bits, valid_bits_offset, num_values, std::move(visit_run)));

      num_values_ -= values_decoded;
      *out_values_decoded = values_decoded;

Review comment (suggestion):
This is the main logical change in this code block: `value_len > len_ - 4` is replaced with the stricter `value_len > estimated_data_length`.

Code after:
                          int64_t valid_bits_offset,
                          typename EncodingTraits<ByteArrayType>::Accumulator* out,
                          int* out_values_decoded) {
    // We're going to decode `num_values - null_count` PLAIN values,
    // and each value has a 4-byte length header that doesn't count for the
    // Arrow binary data length.
    int64_t estimated_data_length = len_ - 4 * (num_values - null_count);
    if (ARROW_PREDICT_FALSE(estimated_data_length < 0)) {
      return Status::Invalid("Invalid or truncated PLAIN-encoded BYTE_ARRAY data");
    }

    auto visit_binary_helper = [&](auto* helper) {
      int values_decoded = 0;

      auto visit_run = [&](int64_t position, int64_t run_length, bool is_valid) {
        if (is_valid) {
          for (int64_t i = 0; i < run_length; ++i) {
            // We ensure `len_` is sufficient thanks to:
            //   1. the initial `estimated_data_length` check above,
            //   2. the running `value_len > estimated_data_length` check below.
            // This precondition follows from those two checks.
            DCHECK_GE(len_, 4);
            auto value_len = SafeLoadAs<int32_t>(data_);
            // This check also ensures that `value_len <= len_ - 4` due to the way
            // `estimated_data_length` is computed.
            if (ARROW_PREDICT_FALSE(value_len < 0 || value_len > estimated_data_length)) {
              return Status::Invalid(
                  "Invalid or truncated PLAIN-encoded BYTE_ARRAY data");
            }
            RETURN_NOT_OK(
                helper->AppendValue(data_ + 4, value_len, estimated_data_length));
            auto increment = value_len + 4;
            data_ += increment;
            len_ -= increment;
            estimated_data_length -= value_len;
            DCHECK_GE(estimated_data_length, 0);
          }
          values_decoded += static_cast<int>(run_length);
          DCHECK_LE(values_decoded, num_values);
          return Status::OK();
        } else {
          return helper->AppendNulls(run_length);
        }
      };

      RETURN_NOT_OK(
          VisitBitRuns(valid_bits, valid_bits_offset, num_values, std::move(visit_run)));

      num_values_ -= values_decoded;
      *out_values_decoded = values_decoded;
      return Status::OK();