Skip to content

Commit fa350c4

Browse files
maltesanderclaude
andcommitted
fix: a year-month interval writes every byte of SQL_INTERVAL_STRUCT
`SQL_YEAR_MONTH_STRUCT` is two SQLUINTEGERs where `SQL_DAY_SECOND_STRUCT` is five, so constructing the union through its year-month arm alone left the last twelve bytes uninitialised. The whole struct is then copied into the application's buffer, so those twelve bytes were driver stack handed to the caller: uninitialised memory an application can read back, and a disclosure however dull its contents. The union is now initialised through its widest arm first and only then overwritten, so every byte of the value is defined whichever target was asked for. Miri found it, as an uninitialised `DaySecond::minute` on the year-month tests, reading through a test helper whose SAFETY comment claimed it read only the arm the target wrote. It did not: building that struct reads both arms eagerly however few the caller goes on to inspect. The comment now records what the soundness actually rests on. The accompanying test asserts the post-condition but is explicitly not the guard: the skipped bytes come from the driver's own stack rather than from the destination buffer, so nothing the test controls decides their value and reverting the fix does not reliably fail it. Miri is what sees this, which is why it runs on every pull request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent be5acd3 commit fa350c4

1 file changed

Lines changed: 88 additions & 15 deletions

File tree

src/column_value.rs

Lines changed: 88 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3111,24 +3111,36 @@ unsafe fn write_interval(
31113111
}
31123112
}
31133113

3114+
// Initialised through the *wider* arm first, always, and only then
3115+
// overwritten with the year-month one where that is the target.
3116+
//
3117+
// `SQL_YEAR_MONTH_STRUCT` is two `SQLUINTEGER`s and
3118+
// `SQL_DAY_SECOND_STRUCT` is five, so writing the union through its
3119+
// year-month arm alone leaves the last twelve bytes uninitialised. This
3120+
// whole struct is then copied into the application's buffer, so those
3121+
// twelve bytes would be driver stack handed to the caller: uninitialised
3122+
// memory an application can read, and a disclosure however uninteresting
3123+
// its contents. Zeroing first costs one store and makes every byte of the
3124+
// value defined.
3125+
let mut interval_value = IntervalUnion {
3126+
day_second: DaySecond::default(),
3127+
};
3128+
if is_year_month_interval(target) {
3129+
// `split_year_month` returns years in `day` and months in `hour`.
3130+
interval_value.year_month = YearMonth {
3131+
year: parts.fields.day,
3132+
month: parts.fields.hour,
3133+
};
3134+
} else {
3135+
interval_value.day_second = parts.fields;
3136+
}
3137+
31143138
let out = IntervalStruct {
31153139
interval_type: target as c_int,
31163140
// `SQL_INTERVAL_STRUCT` carries the sign out of band, in its own field,
31173141
// and every union field is unsigned. 1 is SQL_TRUE.
31183142
interval_sign: i16::from(parts.negative),
3119-
interval_value: if is_year_month_interval(target) {
3120-
// `split_year_month` returns years in `day` and months in `hour`.
3121-
IntervalUnion {
3122-
year_month: YearMonth {
3123-
year: parts.fields.day,
3124-
month: parts.fields.hour,
3125-
},
3126-
}
3127-
} else {
3128-
IntervalUnion {
3129-
day_second: parts.fields,
3130-
}
3131-
},
3143+
interval_value,
31323144
};
31333145
// The return is discarded on purpose: `write_fixed` answers SUCCESS or
31343146
// errors, and the truncation verdict this row reports is decided below.
@@ -5201,8 +5213,18 @@ mod tests {
52015213
)
52025214
}
52035215
.map_err(|e| sqlstate_of_err(&e).to_string())?;
5204-
// SAFETY: both arms are plain-old-data of the same allocation, and the
5205-
// test reads only the one its target wrote.
5216+
// SAFETY: reading *both* arms here is sound only because
5217+
// `write_interval` initialises the union through its widest arm before
5218+
// overwriting either, so all twenty bytes are defined whichever target
5219+
// was asked for. Reading the arm the target did not write yields a
5220+
// meaningless number, not undefined behaviour, and each test asserts on
5221+
// the arm its own target owns.
5222+
//
5223+
// This is a real dependency and not a restatement: the previous
5224+
// "read only the one its target wrote" was wrong, because building this
5225+
// struct reads both eagerly however few the caller goes on to inspect.
5226+
// Miri caught it as an uninitialised `DaySecond::minute` on the
5227+
// year-month tests.
52065228
Ok(WrittenInterval {
52075229
interval_type: out.interval_type,
52085230
interval_sign: out.interval_sign,
@@ -5355,6 +5377,57 @@ mod tests {
53555377
assert_eq!(w.year_month.month, 18);
53565378
}
53575379

5380+
/// Every byte of the union is written, including the twelve a year-month
5381+
/// interval does not use.
5382+
///
5383+
/// `SQL_YEAR_MONTH_STRUCT` is two `SQLUINTEGER`s where
5384+
/// `SQL_DAY_SECOND_STRUCT` is five, and the whole `SQL_INTERVAL_STRUCT` is
5385+
/// copied into the application's buffer whichever arm the target names.
5386+
/// Writing only the year-month arm therefore hands the caller twelve bytes
5387+
/// of driver stack: uninitialised memory an application can read.
5388+
///
5389+
/// **Miri is what actually catches a regression here, not this test.** The
5390+
/// skipped bytes would come from the driver's own stack, not from the
5391+
/// destination buffer, so nothing this test controls decides their value
5392+
/// and they may read back as zero anyway; reverting the fix does not
5393+
/// reliably fail it. Kept because it states and checks the post-condition
5394+
/// cheaply on every run, with the loud version reserved for the tool that
5395+
/// can see an uninitialised read for what it is. The `0xAA` fill is
5396+
/// therefore a lower bound: it proves the writer covered the tail at all.
5397+
#[test]
5398+
fn a_year_month_interval_writes_the_whole_union_not_only_its_own_arm() {
5399+
let v = ColumnValue::IntervalYearMonth {
5400+
years: 1,
5401+
months: 6,
5402+
precision: Interval::YearToMonth,
5403+
};
5404+
// Pre-poisoned, so a byte the writer skips keeps 0xAA and is visible.
5405+
let mut raw = [0xAAu8; size_of::<IntervalStruct>()];
5406+
let mut ind: isize = 0;
5407+
let ret = unsafe {
5408+
write_column_value(
5409+
&v,
5410+
CDataType::IntervalYearToMonth,
5411+
raw.as_mut_ptr().cast::<c_void>(),
5412+
size_of::<IntervalStruct>() as isize,
5413+
&mut ind,
5414+
NumericTarget::UNSPECIFIED,
5415+
)
5416+
}
5417+
.expect("1-06 fits");
5418+
assert_eq!(ret, SqlReturn::SUCCESS);
5419+
5420+
// The union starts after `interval_type` (c_int) and `interval_sign`
5421+
// (i16), rounded up to the union's 4-byte alignment.
5422+
let union_at = size_of::<c_int>() + size_of::<u32>();
5423+
let tail = &raw[union_at + size_of::<YearMonth>()..];
5424+
assert!(
5425+
tail.iter().all(|&b| b == 0),
5426+
"the twelve bytes past the year-month arm must be written, not left \
5427+
as whatever the stack held: {tail:02x?}"
5428+
);
5429+
}
5430+
53585431
/// A negative interval carries its sign in `interval_sign`, out of band,
53595432
/// because every field of the union is unsigned.
53605433
#[test]

0 commit comments

Comments
 (0)