Skip to content

Commit 36940d0

Browse files
kornelskikornelski
authored
Merge pull request #304 from Lucretiel/checked-access-len
2 parents aaba38c + 8fbc5b2 commit 36940d0

File tree

3 files changed

+98
-71
lines changed

3 files changed

+98
-71
lines changed

‎rmp-serde/src/decode.rs‎

Lines changed: 53 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
//! Generic MessagePack deserialization.
22
3+
use std::convert::TryInto;
34
use std::error;
45
use std::fmt::{self, Display, Formatter};
56
use std::io::{self, Cursor, ErrorKind, Read};
7+
use std::num::TryFromIntError;
68
use std::str::{self, Utf8Error};
79

810
use byteorder::{self, ReadBytesExt};
@@ -157,6 +159,13 @@ impl<'a> From<DecodeStringError<'a>> for Error {
157159
}
158160
}
159161

162+
impl From<TryFromIntError> for Error {
163+
#[cold]
164+
fn from(_: TryFromIntError) -> Self {
165+
Error::OutOfRange
166+
}
167+
}
168+
160169
/// A Deserializer that reads bytes from a buffer.
161170
///
162171
/// # Note
@@ -327,8 +336,6 @@ impl<'de, R: ReadSlice<'de>, C: SerializerConfig> Deserializer<R, C> {
327336
}
328337

329338
fn read_128(&mut self) -> Result<[u8; 16], Error> {
330-
use std::convert::TryInto;
331-
332339
let marker = self.take_or_read_marker()?;
333340

334341
if marker != Marker::Bin8 {
@@ -508,35 +515,55 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
508515
Marker::FixStr(len) => Ok(len.into()),
509516
Marker::Str8 => read_u8(&mut self.rd).map(u32::from),
510517
Marker::Str16 => read_u16(&mut self.rd).map(u32::from),
511-
Marker::Str32 | _ => read_u32(&mut self.rd).map(u32::from),
518+
Marker::Str32 => read_u32(&mut self.rd).map(u32::from),
519+
_ => unreachable!()
512520
}?;
513521
self.read_str_data(len, visitor)
514522
}
515523
Marker::FixArray(_) |
516524
Marker::Array16 |
517525
Marker::Array32 => {
518526
let len = match marker {
519-
Marker::FixArray(len) => Ok(len.into()),
520-
Marker::Array16 => read_u16(&mut self.rd).map(u32::from),
521-
Marker::Array32 | _ => read_u32(&mut self.rd).map(u32::from),
522-
}?;
523-
depth_count!(self.depth, visitor.visit_seq(SeqAccess::new(self, len as usize)))
527+
Marker::FixArray(len) => len.into(),
528+
Marker::Array16 => read_u16(&mut self.rd)?.into(),
529+
Marker::Array32 => read_u32(&mut self.rd)?,
530+
_ => unreachable!(),
531+
};
532+
533+
depth_count!(self.depth, {
534+
let mut seq = SeqAccess::new(self, len);
535+
let res = visitor.visit_seq(&mut seq)?;
536+
match seq.left {
537+
0 => Ok(res),
538+
excess => Err(Error::LengthMismatch(len - excess)),
539+
}
540+
})
524541
}
525542
Marker::FixMap(_) |
526543
Marker::Map16 |
527544
Marker::Map32 => {
528545
let len = match marker {
529-
Marker::FixMap(len) => Ok(len.into()),
530-
Marker::Map16 => read_u16(&mut self.rd).map(u32::from),
531-
Marker::Map32 | _ => read_u32(&mut self.rd).map(u32::from),
532-
}?;
533-
depth_count!(self.depth, visitor.visit_map(MapAccess::new(self, len as usize)))
546+
Marker::FixMap(len) => len.into(),
547+
Marker::Map16 => read_u16(&mut self.rd)?.into(),
548+
Marker::Map32 => read_u32(&mut self.rd)?,
549+
_ => unreachable!()
550+
};
551+
552+
depth_count!(self.depth, {
553+
let mut seq = MapAccess::new(self, len);
554+
let res = visitor.visit_map(&mut seq)?;
555+
match seq.left {
556+
0 => Ok(res),
557+
excess => Err(Error::LengthMismatch(len - excess)),
558+
}
559+
})
534560
}
535-
Marker::Bin8 | Marker::Bin16| Marker::Bin32 => {
561+
Marker::Bin8 | Marker::Bin16 | Marker::Bin32 => {
536562
let len = match marker {
537563
Marker::Bin8 => read_u8(&mut self.rd).map(u32::from),
538564
Marker::Bin16 => read_u16(&mut self.rd).map(u32::from),
539-
Marker::Bin32 | _ => read_u32(&mut self.rd).map(u32::from),
565+
Marker::Bin32 => read_u32(&mut self.rd).map(u32::from),
566+
_ => unreachable!()
540567
}?;
541568
match read_bin_data(&mut self.rd, len)? {
542569
Reference::Borrowed(buf) => visitor.visit_borrowed_bytes(buf),
@@ -652,66 +679,22 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
652679
visitor.visit_u128(u128::from_be_bytes(buf))
653680
}
654681

655-
fn deserialize_f32<V>(self, visitor: V) -> Result<V::Value, Self::Error>
656-
where V: Visitor<'de>
657-
{
658-
// This special case allows us to decode some integer types as floats when safe, and
659-
// asked for.
660-
//
661-
// This allows interoperability with msgpack-lite, which performs an optimization of
662-
// storing rounded floats as integers.
663-
let f = match self.take_or_read_marker()? {
664-
Marker::U8 => self.rd.read_data_u8().map(f32::from),
665-
Marker::U16 => self.rd.read_data_u16().map(f32::from),
666-
Marker::I8 => self.rd.read_data_i8().map(f32::from),
667-
Marker::I16 => self.rd.read_data_i16().map(f32::from),
668-
Marker::F32 => self.rd.read_data_f32(),
669-
marker => {
670-
self.marker = Some(marker);
671-
return self.deserialize_any(visitor);
672-
}
673-
}?;
674-
visitor.visit_f32(f)
675-
}
676-
677-
fn deserialize_f64<V>(self, visitor: V) -> Result<V::Value, Self::Error>
678-
where V: Visitor<'de>
679-
{
680-
// This special case allows us to decode some integer types as floats when safe, and
681-
// asked for. This is here to be consistent with 'f32'.
682-
let f = match self.take_or_read_marker()? {
683-
Marker::U8 => self.rd.read_data_u8().map(f64::from),
684-
Marker::U16 => self.rd.read_data_u16().map(f64::from),
685-
Marker::U32 => self.rd.read_data_u32().map(f64::from),
686-
Marker::I8 => self.rd.read_data_i8().map(f64::from),
687-
Marker::I16 => self.rd.read_data_i16().map(f64::from),
688-
Marker::I32 => self.rd.read_data_i32().map(f64::from),
689-
Marker::F32 => self.rd.read_data_f32().map(f64::from),
690-
Marker::F64 => self.rd.read_data_f64(),
691-
marker => {
692-
self.marker = Some(marker);
693-
return self.deserialize_any(visitor);
694-
}
695-
}?;
696-
visitor.visit_f64(f)
697-
}
698-
699682
forward_to_deserialize_any! {
700-
bool u8 u16 u32 u64 i8 i16 i32 i64 char
701-
str string bytes byte_buf unit seq map
702-
struct identifier tuple tuple_struct
703-
ignored_any
683+
bool u8 u16 u32 u64 i8 i16 i32 i64 f32
684+
f64 char str string bytes byte_buf unit
685+
seq map struct identifier tuple
686+
tuple_struct ignored_any
704687
}
705688
}
706689

707690
struct SeqAccess<'a, R, C> {
708691
de: &'a mut Deserializer<R, C>,
709-
left: usize,
692+
left: u32,
710693
}
711694

712695
impl<'a, R: 'a, C> SeqAccess<'a, R, C> {
713696
#[inline]
714-
fn new(de: &'a mut Deserializer<R, C>, len: usize) -> Self {
697+
fn new(de: &'a mut Deserializer<R, C>, len: u32) -> Self {
715698
SeqAccess {
716699
de,
717700
left: len,
@@ -736,17 +719,17 @@ impl<'de, 'a, R: ReadSlice<'de> + 'a, C: SerializerConfig> de::SeqAccess<'de> fo
736719

737720
#[inline(always)]
738721
fn size_hint(&self) -> Option<usize> {
739-
Some(self.left)
722+
self.left.try_into().ok()
740723
}
741724
}
742725

743726
struct MapAccess<'a, R, C> {
744727
de: &'a mut Deserializer<R, C>,
745-
left: usize,
728+
left: u32,
746729
}
747730

748731
impl<'a, R: 'a, C> MapAccess<'a, R, C> {
749-
fn new(de: &'a mut Deserializer<R, C>, len: usize) -> Self {
732+
fn new(de: &'a mut Deserializer<R, C>, len: u32) -> Self {
750733
MapAccess {
751734
de,
752735
left: len,
@@ -763,7 +746,7 @@ impl<'de, 'a, R: ReadSlice<'de> + 'a, C: SerializerConfig> de::MapAccess<'de> fo
763746
{
764747
if self.left > 0 {
765748
self.left -= 1;
766-
Ok(Some(seed.deserialize(&mut *self.de)?))
749+
seed.deserialize(&mut *self.de).map(Some)
767750
} else {
768751
Ok(None)
769752
}
@@ -778,7 +761,7 @@ impl<'de, 'a, R: ReadSlice<'de> + 'a, C: SerializerConfig> de::MapAccess<'de> fo
778761

779762
#[inline(always)]
780763
fn size_hint(&self) -> Option<usize> {
781-
Some(self.left)
764+
self.left.try_into().ok()
782765
}
783766
}
784767

‎rmp-serde/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ impl<'a> RawRef<'a> {
320320
pub fn as_bytes(&self) -> &[u8] {
321321
match self.s {
322322
Ok(s) => s.as_bytes(),
323-
Err(ref err) => err.0,
323+
Err((bytes, _err)) => bytes,
324324
}
325325
}
326326
}

‎rmp-serde/tests/round.rs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,50 @@ fn roundtrip_some() {
565565
assert_roundtrips(Some("hi".to_string()));
566566
}
567567

568+
/// Some types don't fully consume their input SeqAccess, leading to incorrect
569+
/// deserializes.
570+
///
571+
/// https://github.com/3Hren/msgpack-rust/issues/287
572+
#[test]
573+
fn checked_seq_access_len()
574+
{
575+
#[derive(Serialize)]
576+
struct Input {
577+
a: [&'static str; 4],
578+
d: &'static str,
579+
}
580+
581+
#[allow(dead_code)]
582+
#[derive(Deserialize, Debug)]
583+
struct Output {
584+
a: [String; 2],
585+
c: String,
586+
}
587+
588+
let mut buffer = Vec::new();
589+
let mut serializer = Serializer::new(&mut buffer)
590+
.with_binary()
591+
.with_struct_map();
592+
593+
// The bug is basically that Output will successfully deserialize from input
594+
// because the [String; 0] deserializer doesn't drain the SeqAccess, and
595+
// the two fields it leaves behind can then be deserialized into `v`
596+
597+
let data = Input {
598+
a: ["b", "b", "c", "c"],
599+
d: "d",
600+
};
601+
602+
data.serialize(&mut serializer).expect("failed to serialize");
603+
604+
let mut deserializer = rmp_serde::Deserializer::new(
605+
Cursor::new(&buffer)
606+
).with_binary();
607+
608+
Output::deserialize(&mut deserializer)
609+
.expect_err("Input round tripped into Output; this shouldn't happen");
610+
}
611+
568612
#[ignore]
569613
#[test]
570614
fn roundtrip_some_failures() {

0 commit comments

Comments
 (0)