Explooosion-code commented on code in PR #50121:
URL: https://github.com/apache/arrow/pull/50121#discussion_r3664146701
##########
cpp/src/arrow/extension/variant.cc:
##########
@@ -281,56 +281,58 @@ Status VisitObject(const VariantMetadata& metadata, const
uint8_t* data, int64_t
return Status::Invalid("Variant value: truncated object num_fields at
offset ",
offset);
}
- auto num_fields = static_cast<int32_t>(ReadUnsignedLE(data + pos,
num_fields_size));
+ auto num_fields = static_cast<int64_t>(ReadUnsignedLE(data + pos,
num_fields_size));
pos += num_fields_size;
- int64_t field_ids_size = static_cast<int64_t>(num_fields) * field_id_size;
+ int64_t field_ids_size = num_fields * field_id_size;
if (pos + field_ids_size > length) {
return Status::Invalid("Variant value: truncated object field_ids at
offset ",
offset);
}
- std::vector<uint32_t> field_ids(num_fields);
- for (int32_t i = 0; i < num_fields; ++i) {
- field_ids[i] = ReadUnsignedLE(data + pos, field_id_size);
+ std::vector<uint32_t> field_ids(static_cast<size_t>(num_fields));
+ for (int64_t i = 0; i < num_fields; ++i) {
+ field_ids[static_cast<size_t>(i)] = ReadUnsignedLE(data + pos,
field_id_size);
pos += field_id_size;
}
- int64_t offsets_size = (static_cast<int64_t>(num_fields) + 1) *
field_offset_size;
+ int64_t offsets_size = (num_fields + 1) * field_offset_size;
if (pos + offsets_size > length) {
return Status::Invalid("Variant value: truncated object offsets at offset
", offset);
}
- std::vector<uint32_t> value_offsets(num_fields + 1);
- for (int32_t i = 0; i <= num_fields; ++i) {
- value_offsets[i] = ReadUnsignedLE(data + pos, field_offset_size);
+ std::vector<uint32_t> value_offsets(static_cast<size_t>(num_fields + 1));
+ for (int64_t i = 0; i <= num_fields; ++i) {
+ value_offsets[static_cast<size_t>(i)] = ReadUnsignedLE(data + pos,
field_offset_size);
pos += field_offset_size;
}
int64_t data_start = pos;
- int64_t total_data_size = static_cast<int64_t>(value_offsets[num_fields]);
+ int64_t total_data_size =
+ static_cast<int64_t>(value_offsets[static_cast<size_t>(num_fields)]);
if (data_start + total_data_size > length) {
return Status::Invalid("Variant value: object data exceeds buffer at
offset ",
offset);
}
- for (int32_t i = 0; i < num_fields; ++i) {
- if (value_offsets[i] > static_cast<uint32_t>(total_data_size)) {
- return Status::Invalid("Variant value: object field offset ",
value_offsets[i],
- " at index ", i, " exceeds data size ",
total_data_size);
+ for (int64_t i = 0; i < num_fields; ++i) {
+ if (value_offsets[static_cast<size_t>(i)] >
static_cast<uint32_t>(total_data_size)) {
+ return Status::Invalid("Variant value: object field offset ",
+ value_offsets[static_cast<size_t>(i)], " at index
", i,
+ " exceeds data size ", total_data_size);
}
}
- ARROW_RETURN_NOT_OK(visitor->StartObject(num_fields));
+ ARROW_RETURN_NOT_OK(visitor->StartObject(static_cast<int32_t>(num_fields)));
Review Comment:
I'm pretty sure that this problem is still here. `num_field` is bounded by
maximum of uint32_t so casting to int32_t here is invalid and may pass negative
numbers to StartObject (simmilar for StartArray).
In general I think we should be using int64_t for most getters like
`field_id_at` etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
