Copilot commented on code in PR #24006: URL: https://github.com/apache/datafusion/pull/24006#discussion_r3684220350
########## datafusion/datasource/src/proto.rs: ########## @@ -0,0 +1,233 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Protobuf conversions for the file-scan leaf types owned by this crate: +//! [`FileRange`], [`PartitionedFile`] and [`FileGroup`]. +//! +//! These are the single copy of that wire logic. `datafusion-proto`'s +//! `TryFromProto` implementations for the same types are thin shims that +//! delegate here, so the format cannot drift between the central serializer and +//! the per-source `try_to_proto` hooks. +//! +//! None of these conversions need a codec or an encode/decode context: every +//! field is plain data or goes through `datafusion-proto-common`. That is why +//! they are plain [`TryFrom`] impls rather than the `try_to_proto(ctx)` / +//! `try_from_proto(node, ctx)` hooks used for plans, expressions and scan +//! configs: the standard trait can express a conversion that takes nothing but +//! the value, and the orphan rule allows it here because one side of each +//! conversion is a type this crate owns. + +use std::sync::Arc; + +use chrono::{TimeZone, Utc}; +use datafusion_common::{DataFusionError, Result, internal_datafusion_err}; +use datafusion_proto_models::protobuf; +use object_store::ObjectMeta; +use object_store::path::Path; + +use crate::file_groups::FileGroup; +use crate::{FileRange, PartitionedFile}; + +impl TryFrom<&FileRange> for protobuf::FileRange { + type Error = DataFusionError; + + fn try_from(range: &FileRange) -> Result<Self> { + Ok(protobuf::FileRange { + start: range.start, + end: range.end, + }) + } +} + +impl TryFrom<&protobuf::FileRange> for FileRange { + type Error = DataFusionError; + + fn try_from(range: &protobuf::FileRange) -> Result<Self> { + Ok(FileRange { + start: range.start, + end: range.end, + }) + } +} + +impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile { + type Error = DataFusionError; + + fn try_from(file: &PartitionedFile) -> Result<Self> { + let last_modified = file.object_meta.last_modified; + let last_modified_ns = last_modified.timestamp_nanos_opt().ok_or_else(|| { + DataFusionError::Plan(format!( + "Invalid timestamp on PartitionedFile::ObjectMeta: {last_modified}" + )) + })? as u64; Review Comment: The encode/decode path can corrupt timestamps and potentially panic on crafted/invalid protobuf input: (1) encoding casts an `i64` nanos value to `u64` via `as u64`, which will wrap negative timestamps into huge positive values; (2) decoding uses `Utc.timestamp_nanos(file.last_modified_ns as i64)`, which can wrap on `u64 -> i64` overflow and may panic if the resulting `i64` is out of chrono’s valid range. Recommend validating bounds before casting (reject negative nanos on encode; reject `last_modified_ns > i64::MAX` on decode) and using `timestamp_nanos_opt` (or equivalent non-panicking API) to return a structured `DataFusionError` instead of risking a panic/DoS. ########## datafusion/datasource/src/proto.rs: ########## @@ -0,0 +1,233 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Protobuf conversions for the file-scan leaf types owned by this crate: +//! [`FileRange`], [`PartitionedFile`] and [`FileGroup`]. +//! +//! These are the single copy of that wire logic. `datafusion-proto`'s +//! `TryFromProto` implementations for the same types are thin shims that +//! delegate here, so the format cannot drift between the central serializer and +//! the per-source `try_to_proto` hooks. +//! +//! None of these conversions need a codec or an encode/decode context: every +//! field is plain data or goes through `datafusion-proto-common`. That is why +//! they are plain [`TryFrom`] impls rather than the `try_to_proto(ctx)` / +//! `try_from_proto(node, ctx)` hooks used for plans, expressions and scan +//! configs: the standard trait can express a conversion that takes nothing but +//! the value, and the orphan rule allows it here because one side of each +//! conversion is a type this crate owns. + +use std::sync::Arc; + +use chrono::{TimeZone, Utc}; +use datafusion_common::{DataFusionError, Result, internal_datafusion_err}; +use datafusion_proto_models::protobuf; +use object_store::ObjectMeta; +use object_store::path::Path; + +use crate::file_groups::FileGroup; +use crate::{FileRange, PartitionedFile}; + +impl TryFrom<&FileRange> for protobuf::FileRange { + type Error = DataFusionError; + + fn try_from(range: &FileRange) -> Result<Self> { + Ok(protobuf::FileRange { + start: range.start, + end: range.end, + }) + } +} + +impl TryFrom<&protobuf::FileRange> for FileRange { + type Error = DataFusionError; + + fn try_from(range: &protobuf::FileRange) -> Result<Self> { + Ok(FileRange { + start: range.start, + end: range.end, + }) + } +} + +impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile { + type Error = DataFusionError; + + fn try_from(file: &PartitionedFile) -> Result<Self> { + let last_modified = file.object_meta.last_modified; + let last_modified_ns = last_modified.timestamp_nanos_opt().ok_or_else(|| { + DataFusionError::Plan(format!( + "Invalid timestamp on PartitionedFile::ObjectMeta: {last_modified}" + )) + })? as u64; + Ok(protobuf::PartitionedFile { + arrow_schema: file + .arrow_schema + .as_ref() + .map(|s| s.as_ref().try_into()) + .transpose()?, + path: file.object_meta.location.as_ref().to_owned(), + size: file.object_meta.size, + last_modified_ns, + partition_values: file + .partition_values + .iter() + .map(|v| v.try_into()) + .collect::<Result<Vec<_>, _>>()?, + range: file.range.as_ref().map(TryInto::try_into).transpose()?, + statistics: file.statistics.as_ref().map(|s| s.as_ref().into()), + }) + } +} + +impl TryFrom<&protobuf::PartitionedFile> for PartitionedFile { + type Error = DataFusionError; + + fn try_from(file: &protobuf::PartitionedFile) -> Result<Self> { + let mut pf = PartitionedFile::new_from_meta(ObjectMeta { + location: Path::parse(file.path.as_str()).map_err(|e| { + internal_datafusion_err!("Invalid object_store path: {e}") + })?, + last_modified: Utc.timestamp_nanos(file.last_modified_ns as i64), Review Comment: The encode/decode path can corrupt timestamps and potentially panic on crafted/invalid protobuf input: (1) encoding casts an `i64` nanos value to `u64` via `as u64`, which will wrap negative timestamps into huge positive values; (2) decoding uses `Utc.timestamp_nanos(file.last_modified_ns as i64)`, which can wrap on `u64 -> i64` overflow and may panic if the resulting `i64` is out of chrono’s valid range. Recommend validating bounds before casting (reject negative nanos on encode; reject `last_modified_ns > i64::MAX` on decode) and using `timestamp_nanos_opt` (or equivalent non-panicking API) to return a structured `DataFusionError` instead of risking a panic/DoS. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
