gh-141968: Use take_bytes in zipfile._ZipDecrypter - #142240
Conversation
Removes a copy going from bytearray to bytes.
cmaloney
changed the title
|
Does it cause a measurable difference? |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I do not think this has a measurable effect, but at least it does not make it worse.
There was a problem hiding this comment.
I just checked and we do not have any test cases for zip files using zipcrypto which are larger larger than 1K in the zip crate either, btw (see https://github.com/zip-rs/zip2/tree/master/tests/).
As a general comment on this corner of the zip format: I've never seen anyone actually using the zip format's cryptography extensions in the wild, and it has pretty severe cryptographic flaws, so I don't think it's terribly important to optimize. I would personally look to discourage its use.
However, by way of comparison, I can see that we do indeed make sure to avoid copies in our io::Read implementation of encrypted zip file entries in the zip crate: https://github.com/zip-rs/zip2/blob/5fcfad0bdfa587351a2c6f9b322c5dccbfc51aff/src/read.rs#L130-L157
impl<R: Read + ?Sized> Read for CryptoReader<'_, R> {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
match self {
CryptoReader::Plaintext(r) => r.read(buf),
CryptoReader::ZipCrypto(r) => r.read(buf),
#[cfg(feature = "aes-crypto")]
CryptoReader::Aes { reader: r, .. } => r.read(buf),
}
}
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
match self {
CryptoReader::Plaintext(r) => r.read_to_end(buf),
CryptoReader::ZipCrypto(r) => r.read_to_end(buf),
#[cfg(feature = "aes-crypto")]
CryptoReader::Aes { reader: r, .. } => r.read_to_end(buf),
}
}
fn read_to_string(&mut self, buf: &mut String) -> io::Result<usize> {
match self {
CryptoReader::Plaintext(r) => r.read_to_string(buf),
CryptoReader::ZipCrypto(r) => r.read_to_string(buf),
#[cfg(feature = "aes-crypto")]
CryptoReader::Aes { reader: r, .. } => r.read_to_string(buf),
}
}
}So this PR generally seems like the right thing to do. However, if we're looking to optimize zip performance in cpython more generally (i.e. for use cases besides the niche and broken cryptographic support), I would be very interested in contributing to that effort.
3 participants
Removes a copy going from bytearray to bytes.