From a1762f17ef779c46c8715532a3e89d44387e19be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20B=C3=BChler?= Date: Sun, 6 Jun 2021 11:02:45 +0200 Subject: [PATCH 1/3] netlink-proto: optimize Encoder: calculate message length only once fe8cb5e6ff2ec6f6d503f9459eb1f6e20f589edc introduced this when to_bytes() didn't return the size anymore. --- netlink-proto/src/codecs.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/netlink-proto/src/codecs.rs b/netlink-proto/src/codecs.rs index 27535f28..79899fcd 100644 --- a/netlink-proto/src/codecs.rs +++ b/netlink-proto/src/codecs.rs @@ -152,13 +152,12 @@ where let new_len = buf.len() + 2048; buf.resize(new_len, 0); } - let size = msg.buffer_len(); - if buf.remaining_mut() < size { + if buf.remaining_mut() < msg_len { return Err(io::Error::new( io::ErrorKind::Other, format!( "message is {} bytes, but only {} bytes left in the buffer", - size, + msg_len, buf.remaining_mut() ), )); @@ -172,7 +171,7 @@ where // implementation to users, we cannot guarantee // anything. Therefore we have to initialize the buffer // here. - let bytes = &mut buf.chunk_mut()[..size]; + let bytes = &mut buf.chunk_mut()[..msg_len]; for i in 0..bytes.len() { bytes.write_byte(i, 0); } @@ -182,7 +181,7 @@ where msg.serialize(initialized_bytes); trace!(">>> {:?}", msg); - buf.advance_mut(size); + buf.advance_mut(msg_len); } Ok(()) } From 48c212d4bdf02b26f242304c7e96cad0be36ca15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20B=C3=BChler?= Date: Sun, 6 Jun 2021 11:49:25 +0200 Subject: [PATCH 2/3] netlink-proto: fix Encoder allocation - don't allocate in 2048 steps; we know the total size of additional space required (added in d1deb12a03d21038ceb70e0056651701d587a82e) - BytesMut can expand to usize::MAX now; so checking remaining_mut() only prevents a size overflow (can't allocate it anyway). - BytesMut::resize actually changes the size of the buffer and reduces `remaining_mut` - need to actually reserve enough space before taking `&mut buf.chunk_mut()[..msg_len];` --- netlink-proto/src/codecs.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/netlink-proto/src/codecs.rs b/netlink-proto/src/codecs.rs index 79899fcd..5fe1cc32 100644 --- a/netlink-proto/src/codecs.rs +++ b/netlink-proto/src/codecs.rs @@ -147,12 +147,8 @@ where fn encode(&mut self, msg: NetlinkMessage, buf: &mut BytesMut) -> Result<(), Self::Error> { let msg_len = msg.buffer_len(); - // FIXME: we should have a max length for the buffer - while buf.remaining_mut() < msg_len { - let new_len = buf.len() + 2048; - buf.resize(new_len, 0); - } if buf.remaining_mut() < msg_len { + // BytesMut can expand till usize::MAX... unlikely to hit this one. return Err(io::Error::new( io::ErrorKind::Other, format!( @@ -171,6 +167,7 @@ where // implementation to users, we cannot guarantee // anything. Therefore we have to initialize the buffer // here. + buf.reserve(msg_len); let bytes = &mut buf.chunk_mut()[..msg_len]; for i in 0..bytes.len() { bytes.write_byte(i, 0); From 88e09d8f9f2881e36c18af598b4402331d672548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20B=C3=BChler?= Date: Sun, 6 Jun 2021 11:49:43 +0200 Subject: [PATCH 3/3] netlink-proto: get rid of unsafe blocks in Encoder The only semantic difference: on unwind (panic in serialize) the buffer length already is the new one (but the contents were initialized to 0 anyway). --- netlink-proto/src/codecs.rs | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/netlink-proto/src/codecs.rs b/netlink-proto/src/codecs.rs index 5fe1cc32..d5c1af48 100644 --- a/netlink-proto/src/codecs.rs +++ b/netlink-proto/src/codecs.rs @@ -1,4 +1,4 @@ -use std::{fmt::Debug, io, marker::PhantomData, slice}; +use std::{fmt::Debug, io, marker::PhantomData}; use bytes::{BufMut, BytesMut}; use netlink_packet_core::{ @@ -159,27 +159,13 @@ where )); } - // Safety: we initialize the buffer we're passing to - // NetlinkMessage::serialize(). In theory, `serialize()` - // should be safe because it's not supposed to _read_ from - // the buffer, which is potentially - // un-initialized. However, since we delegate the actual - // implementation to users, we cannot guarantee - // anything. Therefore we have to initialize the buffer - // here. - buf.reserve(msg_len); - let bytes = &mut buf.chunk_mut()[..msg_len]; - for i in 0..bytes.len() { - bytes.write_byte(i, 0); - } - - unsafe { - let initialized_bytes = slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len()); - - msg.serialize(initialized_bytes); - trace!(">>> {:?}", msg); - buf.advance_mut(msg_len); - } + // As NetlinkMessage::serialize needs an initialized buffer anyway + // no need for any `unsafe` magic. + let old_len = buf.len(); + let new_len = old_len + msg_len; + buf.resize(new_len, 0); + msg.serialize(&mut buf[old_len..][..msg_len]); + trace!(">>> {:?}", msg); Ok(()) } }