Skip to content

Commit

Permalink
Fix overflow during binary encoding of Intervals (MaterializeInc#10589)
Browse files Browse the repository at this point in the history
In Materialize Intervals internally store the number of months in the
interval and the number of nanoseconds. The pg_wire binary format for
Intervals is 8 bytes for the number of microseconds, followed 4 bytes
for the number of days, followed by 4 bytes for the number of months.
When converting an Interval to the pg_wire binary encoding, Materialize
extracts as many whole days from the nanoseconds as will fit in 4 bytes
(an i32 in Rust). During this it checks if the number of days is larger
than i32::MAX protecting from overflow. However, it never checked if
the number of days is less than i32::MIN making is susceptible to
underflow. This commit fixes that issue.

Additionally when converting from the pg_wire binary format to our
internal Interval representation, we convert the microseconds (which is
stored in an i64) to nanoseconds and then convert the result to an 
i128. If the microseconds are large enough, the conversion to
nanoseconds can overflow the i64. This commit first converts the
microseconds to an i128 before converting to nanoseconds, to avoid an
overflow.
  • Loading branch information
jkosh44 authored Feb 14, 2022
1 parent e49579c commit 19ff5f8
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 28 deletions.
2 changes: 1 addition & 1 deletion src/expr/src/scalar/func/impls/interval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ sqlfunc!(
i = Interval::new(0, 86400, 0)
.unwrap()
.checked_add(
&Interval::new(0, i.dur_as_secs() % (24 * 60 * 60), i.nanoseconds() as i64)
&Interval::new(0, i.dur_as_secs() % (24 * 60 * 60), i.nanoseconds().into())
.unwrap(),
)
.unwrap();
Expand Down
2 changes: 1 addition & 1 deletion src/expr/src/scalar/func/impls/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ sqlfunc!(
Interval::new(
0,
t.num_seconds_from_midnight() as i64,
t.nanosecond() as i64,
t.nanosecond().into(),
)
.map_err(|_| EvalError::IntervalOutOfRange)
}
Expand Down
21 changes: 17 additions & 4 deletions src/pgrepr/src/value/interval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,18 @@ impl ToSql for Interval {
// Our intervals are guaranteed to fit within SQL's min/max intervals,
// so this is compression is guaranteed to be lossless. For details, see
// `repr::scalar::datetime::compute_interval`.
let days = std::cmp::min(self.0.days() as i128, i32::MAX as i128);
let days: i128 = std::cmp::max(
std::cmp::min(self.0.days().into(), i32::MAX.into()),
i32::MIN.into(),
);
let ns = self.0.duration - days * 24 * 60 * 60 * 1_000_000_000;
out.put_i64((ns / 1000) as i64);
out.put_i32(days as i32);
out.put_i64((ns / 1000).try_into().expect(
"bounds checking when creating Intervals should prevent this field from overflowing",
));
out.put_i32(
days.try_into()
.expect("days is bound between i32::MAX and i32::MIN above"),
);
out.put_i32(self.0.months);
Ok(IsNull::No)
}
Expand All @@ -67,7 +75,12 @@ impl<'a> FromSql<'a> for Interval {
let days = raw.read_i32::<NetworkEndian>()?;
let months = raw.read_i32::<NetworkEndian>()?;
Ok(Interval(
ReprInterval::new(months, days as i64 * 24 * 60 * 60, micros * 1000).unwrap(),
ReprInterval::new(
months,
i64::from(days) * 24 * 60 * 60,
i128::from(micros) * 1000,
)
.unwrap(),
))
}

Expand Down
2 changes: 1 addition & 1 deletion src/repr/src/adt/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ impl ParsedDateTime {
}
};

match Interval::new(months, seconds, nanos) {
match Interval::new(months, seconds, nanos.into()) {
Ok(i) => Ok(i),
Err(e) => Err(e.to_string()),
}
Expand Down
10 changes: 5 additions & 5 deletions src/repr/src/adt/interval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ impl Interval {
/// Constructs a new `Interval` with the specified units of time.
///
/// `nanos` in excess of `999_999_999` are carried over into seconds.
pub fn new(months: i32, seconds: i64, nanos: i64) -> Result<Interval, anyhow::Error> {
pub fn new(months: i32, seconds: i64, nanos: i128) -> Result<Interval, anyhow::Error> {
let i = Interval {
months,
duration: i128::from(seconds) * 1_000_000_000 + i128::from(nanos),
duration: i128::from(seconds) * 1_000_000_000 + nanos,
};
// Don't let our duration exceed Postgres' min/max for those same fields,
// equivalent to:
Expand Down Expand Up @@ -92,7 +92,7 @@ impl Interval {
match Self::new(
months,
seconds,
i64::from(self.nanoseconds() + other.nanoseconds()),
(self.nanoseconds() + other.nanoseconds()).into(),
) {
Ok(i) => Some(i),
Err(_) => None,
Expand All @@ -116,7 +116,7 @@ impl Interval {
return None;
}

Self::new(months as i32, seconds as i64, nanos as i64).ok()
Self::new(months as i32, seconds as i64, nanos as i128).ok()
}

pub fn checked_div(&self, other: f64) -> Option<Self> {
Expand All @@ -136,7 +136,7 @@ impl Interval {
return None;
}

Self::new(months as i32, seconds as i64, nanos as i64).ok()
Self::new(months as i32, seconds as i64, nanos as i128).ok()
}

/// Returns the total number of whole seconds in the `Interval`'s duration.
Expand Down
2 changes: 1 addition & 1 deletion src/repr/src/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ impl<'a> From<chrono::Duration> for Datum<'a> {
Interval::new(
0,
duration.num_seconds(),
duration.num_nanoseconds().unwrap_or(0) % 1_000_000_000,
i128::from(duration.num_nanoseconds().unwrap_or(0)) % 1_000_000_000,
)
.unwrap(),
)
Expand Down
64 changes: 64 additions & 0 deletions test/pgtest/binary.pt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,67 @@ BindComplete
DataRow {"fields":["[107, 73, 209, 255, 255, 255, 255, 255, 127, 255, 255, 255, 0, 0, 0, 0]"]}
CommandComplete {"tag":"SELECT 1"}
ReadyForQuery {"status":"I"}

send
Parse {"query": "SELECT INTERVAL '2147483647 months 2147483647 days 2147483647 hours 59 minutes 59.999999 seconds'"}
Bind {"result_formats": [1]}
Execute
Sync
----

until
ReadyForQuery
----
ParseComplete
BindComplete
DataRow {"fields":["[107, 73, 209, 255, 255, 255, 255, 255, 127, 255, 255, 255, 127, 255, 255, 255]"]}
CommandComplete {"tag":"SELECT 1"}
ReadyForQuery {"status":"I"}

send
Parse {"query": "SELECT INTERVAL '-2147483648 days -48 hrs';"}
Bind {"result_formats": [1]}
Execute
Sync
----

until
ReadyForQuery
----
ParseComplete
BindComplete
DataRow {"fields":["[255, 255, 255, 215, 196, 81, 64, 0, 128, 0, 0, 0, 0, 0, 0, 0]"]}
CommandComplete {"tag":"SELECT 1"}
ReadyForQuery {"status":"I"}

send
Parse {"query": "SELECT INTERVAL '-2147483648 days -2147483648 hours -59 minutes -59.999999 seconds'"}
Bind {"result_formats": [1]}
Execute
Sync
----

until
ReadyForQuery
----
ParseComplete
BindComplete
DataRow {"fields":["[148, 182, 45, 255, 41, 108, 92, 1, 128, 0, 0, 0, 0, 0, 0, 0]"]}
CommandComplete {"tag":"SELECT 1"}
ReadyForQuery {"status":"I"}

send
Parse {"query": "SELECT INTERVAL '-2147483648 months -2147483648 days -2147483648 hours -59 minutes -59.999999 seconds'"}
Bind {"result_formats": [1]}
Execute
Sync
----

until
ReadyForQuery
----
ParseComplete
BindComplete
DataRow {"fields":["[148, 182, 45, 255, 41, 108, 92, 1, 128, 0, 0, 0, 128, 0, 0, 0]"]}
CommandComplete {"tag":"SELECT 1"}
ReadyForQuery {"status":"I"}
30 changes: 15 additions & 15 deletions test/sqllogictest/interval.slt
Original file line number Diff line number Diff line change
Expand Up @@ -701,21 +701,21 @@ statement error division by zero
SELECT INTERVAL '1' YEAR / 0

## Largest values
# See #4926
#query T
#SELECT INTERVAL '2147483647 days 2147483647 hours 59 minutes 59.999999 seconds'
#----
#2236962132 days 07:59:59.999999
#
#query T
#SELECT INTERVAL '-2147483647 days -2147483647 hours -59 minutes -59.999999 seconds'
#----
#-2236962132 days -07:59:59.999999
#
#query T
#SELECT INTERVAL '-2147483648 days -2147483648 hours -59 minutes -59.999999 seconds'
#----
#-2236962133 days -08:59:59.999999

query T
SELECT INTERVAL '2147483647 days 2147483647 hours 59 minutes 59.999999 seconds'
----
2236962132 days 07:59:59.999999

query T
SELECT INTERVAL '-2147483647 days -2147483647 hours -59 minutes -59.999999 seconds'
----
-2236962132 days -07:59:59.999999

query T
SELECT INTERVAL '-2147483648 days -2147483648 hours -59 minutes -59.999999 seconds'
----
-2236962133 days -08:59:59.999999

statement error invalid input syntax for type interval: exceeds min/max interval duration
SELECT INTERVAL '2147483647 days 2147483648 hours'
Expand Down

0 comments on commit 19ff5f8

Please sign in to comment.