From ebe402dc9e708a8ed5e5860a7b30ea7826ab52a1 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Mon, 11 Jan 2021 07:27:03 -0500 Subject: [PATCH 1/4] Fix handling of malicious Readers in read_to_end --- library/std/src/io/mod.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index 7ad9e446c5997..0bc0e0e8e83d8 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -390,7 +390,14 @@ where ret = Ok(g.len - start_len); break; } - Ok(n) => g.len += n, + Ok(n) => { + // We can't let g.len overflow which would result in the vec shrinking when the function returns. In + // particular, that could break read_to_string if the shortened buffer doesn't end on a UTF-8 boundary. + // The minimal check would just be a checked_add, but this assert is a bit more precise and should be + // just about the same cost. + assert!(n <= g.buf.len() - g.len); + g.len += n; + } Err(ref e) if e.kind() == ErrorKind::Interrupted => {} Err(e) => { ret = Err(e); From a9ef7983a629fea829955608d67f474f856da53c Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Mon, 11 Jan 2021 07:48:24 -0500 Subject: [PATCH 2/4] clean up control flow --- library/std/src/io/mod.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index 0bc0e0e8e83d8..a054c434d0739 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -366,7 +366,6 @@ where { let start_len = buf.len(); let mut g = Guard { len: buf.len(), buf }; - let ret; loop { if g.len == g.buf.len() { unsafe { @@ -386,10 +385,7 @@ where } match r.read(&mut g.buf[g.len..]) { - Ok(0) => { - ret = Ok(g.len - start_len); - break; - } + Ok(0) => return Ok(g.len - start_len), Ok(n) => { // We can't let g.len overflow which would result in the vec shrinking when the function returns. In // particular, that could break read_to_string if the shortened buffer doesn't end on a UTF-8 boundary. @@ -399,14 +395,9 @@ where g.len += n; } Err(ref e) if e.kind() == ErrorKind::Interrupted => {} - Err(e) => { - ret = Err(e); - break; - } + Err(e) => return Err(e), } } - - ret } pub(crate) fn default_read_vectored(read: F, bufs: &mut [IoSliceMut<'_>]) -> Result From 5cb830397e8493f4bf923b411ec378cd00ce28f9 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Mon, 11 Jan 2021 17:13:50 -0500 Subject: [PATCH 3/4] make check a bit more clear --- library/std/src/io/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index a054c434d0739..af570ac6e30ba 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -384,14 +384,15 @@ where } } - match r.read(&mut g.buf[g.len..]) { + let buf = &mut g.buf[g.len..]; + match r.read(buf) { Ok(0) => return Ok(g.len - start_len), Ok(n) => { // We can't let g.len overflow which would result in the vec shrinking when the function returns. In // particular, that could break read_to_string if the shortened buffer doesn't end on a UTF-8 boundary. // The minimal check would just be a checked_add, but this assert is a bit more precise and should be // just about the same cost. - assert!(n <= g.buf.len() - g.len); + assert!(n <= buf.len()); g.len += n; } Err(ref e) if e.kind() == ErrorKind::Interrupted => {} From e6c07b0628938b0003ecbae0f60b588eebf474aa Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Mon, 11 Jan 2021 17:16:44 -0500 Subject: [PATCH 4/4] clarify docs a bit --- library/std/src/io/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index af570ac6e30ba..f73116ba106cc 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -388,10 +388,9 @@ where match r.read(buf) { Ok(0) => return Ok(g.len - start_len), Ok(n) => { - // We can't let g.len overflow which would result in the vec shrinking when the function returns. In - // particular, that could break read_to_string if the shortened buffer doesn't end on a UTF-8 boundary. - // The minimal check would just be a checked_add, but this assert is a bit more precise and should be - // just about the same cost. + // We can't allow bogus values from read. If it is too large, the returned vec could have its length + // set past its capacity, or if it overflows the vec could be shortened which could create an invalid + // string if this is called via read_to_string. assert!(n <= buf.len()); g.len += n; }