From 7ca9ed62d5c902d93cc45d1a5c6b3b26868353b3 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Sun, 6 Sep 2020 20:01:55 -0700 Subject: [PATCH] Mark `Info::infohash` as potentially lossy `Info::infohash` will discard fields not present in the `Info` struct, and can thus produce an erronous infohash. To try to prevent this function from being called in those circumstances, rename it to `Info::infohash_lossy`, and add a comment explaining why it is dangerous. Embarassingly, `Info::infohash` was called in `TorrentSummary::from_input`, and thus transitively by the torrent show subcommand, where it is definitely not safe, since torrent show is intended to be used with arbitrary torrents. This is now fixed. There was also a build failure caused by a cache issue, so change the cache keys to invalidate the caches. type: fixed --- .github/workflows/build.yaml | 6 +++--- src/info.rs | 11 ++++++++++- src/magnet_link.rs | 5 +++-- src/metainfo.rs | 5 +++-- src/subcommand/torrent/create.rs | 6 ++++-- src/torrent_summary.rs | 9 ++++++--- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index f803f6f..e2f7457 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -49,19 +49,19 @@ jobs: uses: actions/cache@v1 with: path: ~/.cargo/registry - key: ${{ runner.os }}-cargo-registry + key: ${{ runner.os }}-cargo-registry-v0 - name: Cache cargo index uses: actions/cache@v1 with: path: ~/.cargo/git - key: ${{ runner.os }}-cargo-index + key: ${{ runner.os }}-cargo-index-v0 - name: Cache cargo build uses: actions/cache@v1 with: path: target - key: ${{ runner.os }}-cargo-build-target + key: ${{ runner.os }}-cargo-build-target-v0 - name: Install Stable uses: actions-rs/toolchain@v1 diff --git a/src/info.rs b/src/info.rs index 3c8fda8..e2928c6 100644 --- a/src/info.rs +++ b/src/info.rs @@ -34,7 +34,16 @@ impl Info { self.mode.content_size() } - pub(crate) fn infohash(&self) -> Result { + /// This function is potentially lossy. If an arbitrary torrent info + /// dictionary is deserialized into an `Info` struct, extra fields not present + /// on the struct will be discarded. If `infohash_lossy` is then called on + /// the resultant struct, those fields will not contribute to the infohash, + /// which will thus be different from that of the original torrent. + /// + /// It will not be lossy if no extra fields are present in the original + /// torrent. So, it is safe to call on torrents that have just been created + /// and are still in memory, and thus are known to have no extra fields. + pub(crate) fn infohash_lossy(&self) -> Result { let encoded = bendy::serde::ser::to_bytes(self).context(error::InfoSerialize)?; Ok(Infohash::from_bencoded_info_dict(&encoded)) } diff --git a/src/magnet_link.rs b/src/magnet_link.rs index 40fbdbd..f7b1993 100644 --- a/src/magnet_link.rs +++ b/src/magnet_link.rs @@ -9,8 +9,9 @@ pub(crate) struct MagnetLink { } impl MagnetLink { - pub(crate) fn from_metainfo(metainfo: &Metainfo) -> Result { - let mut link = Self::with_infohash(metainfo.infohash()?); + /// See `Info::infohash_lossy` for details on when this function is lossy. + pub(crate) fn from_metainfo_lossy(metainfo: &Metainfo) -> Result { + let mut link = Self::with_infohash(metainfo.infohash_lossy()?); link.set_name(metainfo.info.name.clone()); diff --git a/src/metainfo.rs b/src/metainfo.rs index 0b515b1..a90b223 100644 --- a/src/metainfo.rs +++ b/src/metainfo.rs @@ -114,8 +114,9 @@ impl Metainfo { }) } - pub(crate) fn infohash(&self) -> Result { - self.info.infohash() + /// See `Info::infohash_lossy` for details on when this function is lossy. + pub(crate) fn infohash_lossy(&self) -> Result { + self.info.infohash_lossy() } #[cfg(test)] diff --git a/src/subcommand/torrent/create.rs b/src/subcommand/torrent/create.rs index ef6a6f8..3c11a6e 100644 --- a/src/subcommand/torrent/create.rs +++ b/src/subcommand/torrent/create.rs @@ -454,11 +454,13 @@ impl Create { } if self.show { - TorrentSummary::from_metainfo(metainfo.clone())?.write(env)?; + // We just created this torrent, so no extra fields have been discarded. + TorrentSummary::from_metainfo_lossy(metainfo.clone())?.write(env)?; } if self.print_magnet_link { - let mut link = MagnetLink::from_metainfo(&metainfo)?; + // We just created this torrent, so no extra fields have been discarded. + let mut link = MagnetLink::from_metainfo_lossy(&metainfo)?; for peer in self.peers { link.add_peer(peer); } diff --git a/src/torrent_summary.rs b/src/torrent_summary.rs index 3c3281e..2081e78 100644 --- a/src/torrent_summary.rs +++ b/src/torrent_summary.rs @@ -15,17 +15,20 @@ impl TorrentSummary { } } - pub(crate) fn from_metainfo(metainfo: Metainfo) -> Result { + /// See `Info::infohash_lossy` for details on when this function is lossy. + pub(crate) fn from_metainfo_lossy(metainfo: Metainfo) -> Result { let bytes = metainfo.serialize()?; let size = Bytes(bytes.len().into_u64()); - let infohash = metainfo.infohash()?; + let infohash = metainfo.infohash_lossy()?; Ok(Self::new(metainfo, infohash, size)) } pub(crate) fn from_input(input: &Input) -> Result { let metainfo = Metainfo::from_input(input)?; + let infohash = Infohash::from_input(input)?; + let size = Bytes(input.data().len().into_u64()); - Ok(Self::from_metainfo(metainfo)?) + Ok(Self::new(metainfo, infohash, size)) } pub(crate) fn write(&self, env: &mut Env) -> Result<()> {