History

address auto-review

GPT 5.4 committed 6 months ago

Commit 7429b151 parent e64e3b815 files · +170 −73Browse files

Unified · Split

15 files
  1. gix-bitmap/src/ewah.rs +15 −2
  2. gix-bitmap/tests/bitmap.rs +21 −0
  3. gix-features/src/decode.rs +1 −1
  4. gix-odb/src/store_impls/loose/find.rs +4 −4
  5. gix-pack/fuzz/fuzz_targets/data_file.rs +1 −1
  6. gix-pack/fuzz/fuzz_targets/multi_index_file.rs +1 −1
  7. gix-pack/src/data/delta.rs +5 −5
  8. gix-pack/src/data/entry/decode.rs +2 −2
  9. gix-pack/src/index/init.rs +4 −4
  10. gix-pack/src/index/write/mod.rs +1 −1
  11. gix-pack/src/multi_index/chunk.rs +3 −3
  12. gix-pack/src/multi_index/verify.rs +7 −1
  13. gix-pack/tests/pack/multi_index/mod.rs +1 −1
  14. gix-transport/src/client/blocking_io/http/redirect.rs +98 −40
  15. gix-transport/src/client/blocking_io/http/reqwest/remote.rs +6 −7
gix-bitmap/src/ewah.rs+15−2
@@ -62,12 +62,25 @@ mod access { · 61 unmodified lines
6262 index += 1;
6363 }
6464 } else {
65− index += usize::try_from(rlw_running_len_bits(word)).ok()?;
65+ let len = usize::try_from(rlw_running_len_bits(word)).ok()?;
66+ let end = index.checked_add(len)?;
67+ if end > num_bits {
68+ return None;
69+ }
70+ index = end;
6671 }
6772
6873 for _ in 0..rlw_literal_words(word) {
6974 let word = iter.next()?;
70− for bit_index in 0..64 {
75+ let remaining = num_bits.checked_sub(index)?;
76+ if remaining == 0 {
77+ return None;
78+ }
79+ let bits_in_word = remaining.min(64);
80+ if bits_in_word < 64 && (word >> bits_in_word) != 0 {
81+ return None;
82+ }
83+ for bit_index in 0..bits_in_word {
7184 if word & (1 << bit_index) != 0 {
7285 f(index)?;
7386 }
gix-bitmap/tests/bitmap.rs+21−0
@@ -22,6 +22,27 @@ mod fuzzed { · 21 unmodified lines
2222 );
2323 }
2424
25+ #[test]
26+ fn non_zero_padding_bits_in_last_literal_word_are_rejected() {
27+ let mut data = Vec::new();
28+ // One logical bit, two compressed words, then RLW position 0.
29+ data.extend_from_slice(&1u32.to_be_bytes());
30+ data.extend_from_slice(&2u32.to_be_bytes());
31+ // The only in-range bit is bit 0 and it is unset; bit 33 is set only in padding.
32+ data.extend_from_slice(&(1u64 << 33).to_be_bytes());
33+ data.extend_from_slice(&2u64.to_be_bytes());
34+ data.extend_from_slice(&0u32.to_be_bytes());
35+
36+ let (bitmap, rest) = gix_bitmap::ewah::decode(&data).expect("fixture must decode");
37+
38+ assert!(rest.is_empty(), "fixture should be fully consumed");
39+ assert_eq!(
40+ bitmap.for_each_set_bit(|_| Some(())),
41+ None,
42+ "set bits outside the declared bit length must be rejected"
43+ );
44+ }
45+
2546 fn artifact_paths(target: &str) -> Vec<std::path::PathBuf> {
2647 std::fs::read_dir(
2748 std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"))
gix-features/src/decode.rs+1−1
@@ -16,7 +16,7 @@ pub fn leb64_from_read(mut r: impl Read) -> Result<(u64, usize), std::io::Error> · 15 unmodified lines
1616 .checked_add(1)
1717 .and_then(|value| value.checked_shl(7))
1818 .and_then(|value| value.checked_add(u64::from(c) & 0x7f))
19− .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidData, "ofs-delta distance overflowed"))?;
19+ .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidData, "LEB64 value overflowed"))?;
2020 }
2121 Ok((value, i))
2222 }
gix-odb/src/store_impls/loose/find.rs+4−4
@@ -29,7 +29,7 @@ pub enum Error { · 28 unmodified lines
2929
3030 /// Object lookup
3131 impl Store {
32− const MAP_ACTION: &'static str = "open";
32+ const OPEN_OR_MAP_ACTION: &'static str = "open or map";
3333
3434 /// Returns true if the given id is contained in our repository.
3535 pub fn contains(&self, id: &gix_hash::oid) -> bool {
@@ -200,7 +200,7 @@ impl Store { · 164 unmodified lines
200200 zlib::stream::inflate::read(&mut input, &mut inflate.state, &mut out[decompressed_body_prefix_len..])
201201 .map_err(|e| Error::Io {
202202 source: e,
203− action: "deflate",
203+ action: "inflate",
204204 path: path.to_owned(),
205205 })?;
206206
@@ -232,7 +232,7 @@ impl Store { · 25 unmodified lines
232232 Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
233233 Err(err) => {
234234 return Err(Error::Io {
235− action: Self::MAP_ACTION,
235+ action: Self::OPEN_OR_MAP_ACTION,
236236 source: err,
237237 path: path.to_owned(),
238238 })
@@ -242,7 +242,7 @@ impl Store { · 3 unmodified lines
242242 if map.is_empty() {
243243 return Err(Error::Io {
244244 source: io::Error::other("empty loose object file"),
245− action: Self::MAP_ACTION,
245+ action: Self::OPEN_OR_MAP_ACTION,
246246 path: path.to_owned(),
247247 });
248248 }
gix-pack/fuzz/fuzz_targets/data_file.rs+1−1
@@ -3,7 +3,7 @@ · 2 unmodified lines
33 use anyhow::Result;
44 use gix_features::{progress, zlib};
55 use gix_pack::{cache, data};
6−use gix_pack_fuzz::{virtual_path, interrupt_flag};
6+use gix_pack_fuzz::{interrupt_flag, virtual_path};
77 use libfuzzer_sys::fuzz_target;
88 use std::hint::black_box;
99
gix-pack/fuzz/fuzz_targets/multi_index_file.rs+1−1
@@ -4,7 +4,7 @@ use anyhow::Result; · 3 unmodified lines
44 use gix_features::progress;
55 use gix_hash::Prefix;
66 use gix_pack::multi_index;
7−use gix_pack_fuzz::{interrupt_flag, virtual_path, empty_candidates};
7+use gix_pack_fuzz::{empty_candidates, interrupt_flag, virtual_path};
88 use libfuzzer_sys::fuzz_target;
99 use std::hint::black_box;
1010
gix-pack/src/data/delta.rs+5−5
@@ -110,11 +110,11 @@ pub(crate) fn apply(base: &[u8], mut target: &mut [u8], data: &[u8]) -> Result<( · 109 unmodified lines
110110 }
111111 }
112112 }
113− if i != data.len() {
114− return Err(apply::Error::Corrupt {
115− message: "delta instructions were not consumed completely",
116− });
117− }
113+ debug_assert_eq!(
114+ i,
115+ data.len(),
116+ "delta instructions were not consumed completely, should be impossible"
117+ );
118118 if !target.is_empty() {
119119 return Err(apply::Error::Corrupt {
120120 message: "delta instructions produced fewer bytes than promised",
gix-pack/src/data/entry/decode.rs+2−2
@@ -11,7 +11,7 @@ use crate::data; · 10 unmodified lines
1111 pub enum Error {
1212 #[error("Object type {type_id} is unsupported")]
1313 UnsupportedType { type_id: u8 },
14− #[error("Pack entry is truncated while reading {message}")]
14+ #[error("Pack entry is truncated: {message}")]
1515 Corrupt { message: &'static str },
1616 #[error("Pack entry header value overflowed while decoding")]
1717 Overflow,
@@ -142,7 +142,7 @@ fn parse_header_info(data: &[u8]) -> Result<(u8, u64, usize), Error> { · 124 unmodified lines
142142 let mut shift = 4u32;
143143 while c & 0b1000_0000 != 0 {
144144 c = *data.get(i).ok_or(Error::Corrupt {
145− message: "Pack entry header continuation byte is missing",
145+ message: "pack entry header continuation byte",
146146 })?;
147147 i += 1;
148148 let component = u64::from(c & 0b0111_1111).checked_shl(shift).ok_or(Error::Overflow)?;
gix-pack/src/index/init.rs+4−4
@@ -153,14 +153,15 @@ fn validate_size(data: &[u8], kind: Version, num_objects: u32, hash_len: usize) · 152 unmodified lines
153153 ),
154154 });
155155 }
156− let large_offset_indices: Vec<_> = data[offset32_start..offset32_end]
156+ let (large_offsets, max_large_offset_index) = data[offset32_start..offset32_end]
157157 .chunks_exact(N32_SIZE)
158158 .filter_map(|offset| {
159159 let offset = crate::read_u32(offset);
160160 (offset & (1 << 31) != 0).then_some((offset ^ (1 << 31)) as usize)
161161 })
162− .collect();
163− let large_offsets = large_offset_indices.len();
162+ .fold((0usize, 0usize), |(count, max_index), index| {
163+ (count + 1, max_index.max(index))
164+ });
164165 v2_header_size
165166 .checked_add(oid_bytes)
166167 .and_then(|size| size.checked_add(table_bytes))
@@ -171,7 +172,6 @@ fn validate_size(data: &[u8], kind: Version, num_objects: u32, hash_len: usize) · 4 unmodified lines
171172 message: "Pack index size overflowed while validating version 2 layout".into(),
172173 })
173174 .and_then(|expected_size| {
174− let max_large_offset_index = large_offset_indices.into_iter().max().unwrap_or(0);
175175 if large_offsets > 0 && max_large_offset_index >= large_offsets {
176176 return Err(Error::Corrupt {
177177 message: format!(
gix-pack/src/index/write/mod.rs+1−1
@@ -22,7 +22,7 @@ pub struct Outcome { · 21 unmodified lines
2222 pub num_objects: u32,
2323 }
2424
25−/// The progress ids used in [`write_data_iter_from_stream()`][crate::index::write_data_iter_to_stream()].
25+/// The progress ids used in [`write_data_iter_to_stream()`][crate::index::write_data_iter_to_stream()].
2626 ///
2727 /// Use this information to selectively extract the progress of interest in case the parent application has custom visualization.
2828 #[derive(Debug, Copy, Clone)]
gix-pack/src/multi_index/chunk.rs+3−3
@@ -203,7 +203,7 @@ pub mod lookup { · 202 unmodified lines
203203
204204 /// Return true if the size of the `offset` range seems to match for a `hash` of the given kind and the amount of objects.
205205 pub fn is_valid(offset: &Range<usize>, hash: gix_hash::Kind, num_objects: u32) -> bool {
206− (offset.end - offset.start) == num_objects as usize * hash.len_in_bytes()
206+ (offset.end - offset.start) == (num_objects as usize).saturating_mul(hash.len_in_bytes())
207207 }
208208 }
209209
@@ -251,10 +251,10 @@ pub mod offsets { · 41 unmodified lines
251251 Ok(())
252252 }
253253
254− /// Returns true if the `offset` range seems to match the size required for `num_objects`.
254+ /// Returns true if the `offset` range seems to match the size required for the untrusted `num_objects`.
255255 pub fn is_valid(offset: &Range<usize>, num_objects: u32) -> bool {
256256 let entry_size = 4 /* pack-id */ + 4 /* pack-offset */;
257− (offset.end - offset.start) == num_objects as usize * entry_size
257+ (offset.end - offset.start) == (num_objects as usize).saturating_mul(entry_size)
258258 }
259259 }
260260
gix-pack/src/multi_index/verify.rs+7−1
@@ -34,6 +34,8 @@ pub mod integrity { · 33 unmodified lines
3434 Fan { index: usize },
3535 #[error("The multi-index claims to have no objects")]
3636 Empty,
37+ #[error("The multi-index path '{path}' has no parent directory")]
38+ InvalidPath { path: std::path::PathBuf },
3739 #[error("Interrupted")]
3840 Interrupted,
3941 }
@@ -142,7 +144,11 @@ where · 102 unmodified lines
142144 C: crate::cache::DecodeEntry,
143145 F: Fn() -> C + Send + Clone,
144146 {
145− let parent = self.path.parent().unwrap_or_else(|| std::path::Path::new(""));
147+ let parent = self.path.parent().ok_or_else(|| {
148+ index::traverse::Error::Processor(integrity::Error::InvalidPath {
149+ path: self.path.clone(),
150+ })
151+ })?;
146152
147153 let actual_index_checksum = self
148154 .verify_checksum(
gix-pack/tests/pack/multi_index/mod.rs+1−1
@@ -6,7 +6,7 @@ fn multi_index() -> (File, PathBuf) { · 5 unmodified lines
66 let path = crate::scripted_fixture_read_only("make_pack_gen_repo_multi_index.sh")
77 .expect("test fixture exists")
88 .join(".git/objects/pack/multi-pack-index");
9− let file = gix_pack::multi_index::File::at(&path, None).unwrap();
9+ let file = gix_pack::multi_index::File::at(&path, None).expect("multi-index fixture opens");
1010 (file, path)
1111 }
1212
gix-transport/src/client/blocking_io/http/redirect.rs+98−40
@@ -6,21 +6,36 @@ pub struct Error { · 5 unmodified lines
66 expected_url: String,
77 }
88
9−fn shares_authority(lhs: &str, rhs: &str) -> bool {
10− let Ok(lhs) = gix_url::parse(lhs.into()) else {
9+pub(crate) fn shares_authority_or_upgrades_scheme(redirect_url: &str, original_url: &str) -> bool {
10+ let Ok(redirect_url) = gix_url::parse(redirect_url.into()) else {
1111 return false;
1212 };
13− let Ok(rhs) = gix_url::parse(rhs.into()) else {
13+ let Ok(original_url) = gix_url::parse(original_url.into()) else {
1414 return false;
1515 };
16− lhs.scheme == rhs.scheme && lhs.host == rhs.host && lhs.port_or_default() == rhs.port_or_default()
16+
17+ if redirect_url.host != original_url.host {
18+ return false;
19+ }
20+
21+ if redirect_url.scheme == original_url.scheme {
22+ return redirect_url.port_or_default() == original_url.port_or_default();
23+ }
24+
25+ if original_url.scheme == gix_url::Scheme::Http && redirect_url.scheme == gix_url::Scheme::Https {
26+ let original_port = original_url.port_or_default();
27+ let redirect_port = redirect_url.port_or_default();
28+ return original_port == redirect_port || matches!((original_port, redirect_port), (Some(80), Some(443)));
29+ }
30+
31+ false
1732 }
1833
1934 pub(crate) fn base_url(redirect_url: &str, base_url: &str, url: String) -> Result<String, Error> {
2035 let tail = url
2136 .strip_prefix(base_url)
2237 .expect("BUG: caller assures `base_url` is subset of `url`");
23− if !shares_authority(redirect_url, base_url) {
38+ if !shares_authority_or_upgrades_scheme(redirect_url, base_url) {
2439 return Err(Error {
2540 redirect_url: redirect_url.into(),
2641 expected_url: url,
@@ -62,46 +77,89 @@ mod tests { · 35 unmodified lines
6277 );
6378 }
6479
80+ #[test]
81+ fn base_url_allows_same_host_scheme_upgrade() {
82+ assert_eq!(
83+ base_url(
84+ "https://original/b/info/refs?hi",
85+ "http://original/a",
86+ "http://original/a/info/refs?hi".into()
87+ )
88+ .unwrap(),
89+ "https://original/b"
90+ );
91+ }
92+
6593 #[test]
6694 fn base_url_rejects_authority_changes() {
67− assert!(base_url(
68− "http://original/b/info/refs?hi",
69− "https://original/a",
70− "https://original/a/info/refs?hi".into()
71− )
72− .is_err());
73− assert!(base_url(
74− "https://redirected.org/b/info/refs?hi",
75− "https://original/a",
76− "https://original/a/info/refs?hi".into()
77− )
78− .is_err());
79− assert!(base_url(
80− "https://original:444/b/info/refs?hi",
81− "https://original/a",
82− "https://original/a/info/refs?hi".into()
83− )
84− .is_err());
95+ assert!(
96+ base_url(
97+ "http://original/b/info/refs?hi",
98+ "https://original/a",
99+ "https://original/a/info/refs?hi".into()
100+ )
101+ .is_err(),
102+ "downgrading from https to http must be rejected"
103+ );
104+ assert!(
105+ base_url(
106+ "https://redirected.org/b/info/refs?hi",
107+ "https://original/a",
108+ "https://original/a/info/refs?hi".into()
109+ )
110+ .is_err(),
111+ "changing the host must be rejected"
112+ );
113+ assert!(
114+ base_url(
115+ "https://original:444/b/info/refs?hi",
116+ "https://original/a",
117+ "https://original/a/info/refs?hi".into()
118+ )
119+ .is_err(),
120+ "changing the port on https must be rejected"
121+ );
122+ assert!(
123+ base_url(
124+ "https://original:444/b/info/refs?hi",
125+ "http://original/a",
126+ "http://original/a/info/refs?hi".into()
127+ )
128+ .is_err(),
129+ "upgrading the scheme does not allow changing the port"
130+ );
85131 }
86132
87133 #[test]
88− fn shares_authority_complete() {
89− assert!(shares_authority(
90− "https://original/b/info/refs?hi",
91− "https://original/a"
92− ));
93− assert!(!shares_authority(
94− "http://original/b/info/refs?hi",
95− "https://original/a"
96− ));
97− assert!(!shares_authority(
98− "https://redirected.org/b/info/refs?hi",
99− "https://original/a"
100− ));
101− assert!(!shares_authority(
102− "https://original:444/b/info/refs?hi",
103− "https://original/a"
104− ));
134+ fn shares_authority_or_upgrades_scheme_complete() {
135+ assert!(
136+ shares_authority_or_upgrades_scheme("https://original/b/info/refs?hi", "https://original/a"),
137+ "keeping the same https authority must be allowed"
138+ );
139+ assert!(
140+ shares_authority_or_upgrades_scheme("https://original/b/info/refs?hi", "http://original/a"),
141+ "upgrading from http to https on the same host must be allowed"
142+ );
143+ assert!(
144+ shares_authority_or_upgrades_scheme("https://original:8080/b/info/refs?hi", "http://original:8080/a"),
145+ "upgrading from http to https on the same host and port must be allowed"
146+ );
147+ assert!(
148+ !shares_authority_or_upgrades_scheme("http://original/b/info/refs?hi", "https://original/a"),
149+ "downgrading from https to http must be rejected"
150+ );
151+ assert!(
152+ !shares_authority_or_upgrades_scheme("https://redirected.org/b/info/refs?hi", "https://original/a"),
153+ "changing the host must be rejected"
154+ );
155+ assert!(
156+ !shares_authority_or_upgrades_scheme("https://original:444/b/info/refs?hi", "https://original/a"),
157+ "changing the port on https must be rejected"
158+ );
159+ assert!(
160+ !shares_authority_or_upgrades_scheme("https://original:444/b/info/refs?hi", "http://original/a"),
161+ "upgrading the scheme does not allow changing the port"
162+ );
105163 }
106164
107165 #[test]
gix-transport/src/client/blocking_io/http/reqwest/remote.rs+6−7
@@ -41,12 +41,6 @@ impl Default for Remote { · 40 unmodified lines
4141 let (req_send, req_recv) = std::sync::mpsc::sync_channel(0);
4242 let (res_send, res_recv) = std::sync::mpsc::sync_channel(0);
4343 let handle = std::thread::spawn(move || -> Result<(), Error> {
44− fn shares_authority(lhs: &reqwest::Url, rhs: &reqwest::Url) -> bool {
45− lhs.scheme() == rhs.scheme()
46− && lhs.host_str() == rhs.host_str()
47− && lhs.port_or_known_default() == rhs.port_or_known_default()
48− }
49−
5044 let mut follow = None;
5145 let mut redirected_base_url = None::<String>;
5246 let allow_redirects = Arc::new(atomic::AtomicBool::new(false));
@@ -64,7 +58,12 @@ impl Default for Remote { · 11 unmodified lines
6458 let prev_urls = attempt.previous();
6559
6660 match prev_urls.first() {
67− Some(prev_url) if !shares_authority(prev_url, curr_url) => {
61+ Some(prev_url)
62+ if !redirect::shares_authority_or_upgrades_scheme(
63+ curr_url.as_str(),
64+ prev_url.as_str(),
65+ ) =>
66+ {
6867 // git does not want to be redirected to a different host.
6968 attempt.stop()
7069 }