Fixed panic when canonicalizing not-a-base-url, fixes #4394

diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs
index 4841b5e4ea6c92a7706d964f28f090a7eb0c993f..7dc9c5c9831c14cfabf68ac1b5213f7ffdbf642b 100644 (file)
--- a/src/bin/git_checkout.rs
+++ b/src/bin/git_checkout.rs
@@ -40,9 +40,9 @@ pub fn execute(options: Options, config: &Config) -> CliResult {
     let url = url.to_url()?;
 
     let reference = GitReference::Branch(reference.clone());
-    let source_id = SourceId::for_git(&url, reference);
+    let source_id = SourceId::for_git(&url, reference)?;
 
-    let mut source = GitSource::new(&source_id, config);
+    let mut source = GitSource::new(&source_id, config)?;
 
     source.update()?;
 

diff --git a/src/bin/install.rs b/src/bin/install.rs
index 7aaca4d484eb8a77893c0f1642c35ad7f3d9f350..12518926ca9158741989f0b08380dfa6d11742e5 100644 (file)
--- a/src/bin/install.rs
+++ b/src/bin/install.rs
@@ -136,7 +136,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult {
         } else {
             GitReference::Branch("master".to_string())
         };
-        SourceId::for_git(&url, gitref)
+        SourceId::for_git(&url, gitref)?
     } else if let Some(path) = options.flag_path {
         SourceId::for_path(&config.cwd().join(path))?
     } else if options.arg_crate.is_empty() {

diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs
index 7035ef5db0f53b9a983784fa6bfe55068eb5abd5..321848c91c3f37578c22429ad79b05b071799432 100644 (file)
--- a/src/cargo/core/package_id.rs
+++ b/src/cargo/core/package_id.rs
@@ -180,7 +180,7 @@ mod tests {
     #[test]
     fn invalid_version_handled_nicely() {
         let loc = CRATES_IO.to_url().unwrap();
-        let repo = SourceId::for_registry(&loc);
+        let repo = SourceId::for_registry(&loc).unwrap();
 
         assert!(PackageId::new("foo", "1.0", &repo).is_err());
         assert!(PackageId::new("foo", "1", &repo).is_err());

diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs
index c24b48a365d00ca4f634228954ce6c6609684c26..3136e3fd6feeaf0ef9f747549cf46fee225aea11 100644 (file)
--- a/src/cargo/core/package_id_spec.rs
+++ b/src/cargo/core/package_id_spec.rs
@@ -270,7 +270,7 @@ mod tests {
     #[test]
     fn matching() {
         let url = Url::parse("http://example.com").unwrap();
-        let sid = SourceId::for_registry(&url);
+        let sid = SourceId::for_registry(&url).unwrap();
         let foo = PackageId::new("foo", "1.2.3", &sid).unwrap();
         let bar = PackageId::new("bar", "1.2.3", &sid).unwrap();
 

diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs
index 98730e3f081831cff70d0bc71c16d1145f4cf98f..5d1347238c973ba724730528d0b16dd0c8bd2852 100644 (file)
--- a/src/cargo/core/source.rs
+++ b/src/cargo/core/source.rs
@@ -114,15 +114,16 @@ struct SourceIdInner {
 }
 
 impl SourceId {
-    fn new(kind: Kind, url: Url) -> SourceId {
-        SourceId {
+    fn new(kind: Kind, url: Url) -> CargoResult<SourceId> {
+        let source_id = SourceId {
             inner: Arc::new(SourceIdInner {
                 kind: kind,
-                canonical_url: git::canonicalize_url(&url),
+                canonical_url: git::canonicalize_url(&url)?,
                 url: url,
                 precise: None,
             }),
-        }
+        };
+        Ok(source_id)
     }
 
     /// Parses a source URL and returns the corresponding ID.
@@ -158,16 +159,16 @@ impl SourceId {
                 let precise = url.fragment().map(|s| s.to_owned());
                 url.set_fragment(None);
                 url.set_query(None);
-                Ok(SourceId::for_git(&url, reference).with_precise(precise))
+                Ok(SourceId::for_git(&url, reference)?.with_precise(precise))
             },
             "registry" => {
                 let url = url.to_url()?;
-                Ok(SourceId::new(Kind::Registry, url)
+                Ok(SourceId::new(Kind::Registry, url)?
                             .with_precise(Some("locked".to_string())))
             }
             "path" => {
                 let url = url.to_url()?;
-                Ok(SourceId::new(Kind::Path, url))
+                SourceId::new(Kind::Path, url)
             }
             kind => Err(format!("unsupported source protocol: {}", kind).into())
         }
@@ -180,25 +181,25 @@ impl SourceId {
     // Pass absolute path
     pub fn for_path(path: &Path) -> CargoResult<SourceId> {
         let url = path.to_url()?;
-        Ok(SourceId::new(Kind::Path, url))
+        SourceId::new(Kind::Path, url)
     }
 
-    pub fn for_git(url: &Url, reference: GitReference) -> SourceId {
+    pub fn for_git(url: &Url, reference: GitReference) -> CargoResult<SourceId> {
         SourceId::new(Kind::Git(reference), url.clone())
     }
 
-    pub fn for_registry(url: &Url) -> SourceId {
+    pub fn for_registry(url: &Url) -> CargoResult<SourceId> {
         SourceId::new(Kind::Registry, url.clone())
     }
 
     pub fn for_local_registry(path: &Path) -> CargoResult<SourceId> {
         let url = path.to_url()?;
-        Ok(SourceId::new(Kind::LocalRegistry, url))
+        SourceId::new(Kind::LocalRegistry, url)
     }
 
     pub fn for_directory(path: &Path) -> CargoResult<SourceId> {
         let url = path.to_url()?;
-        Ok(SourceId::new(Kind::Directory, url))
+        SourceId::new(Kind::Directory, url)
     }
 
     /// Returns the `SourceId` corresponding to the main repository.
@@ -220,7 +221,7 @@ impl SourceId {
             CRATES_IO
         };
         let url = url.to_url()?;
-        Ok(SourceId::for_registry(&url))
+        SourceId::for_registry(&url)
     }
 
     pub fn url(&self) -> &Url {
@@ -241,31 +242,31 @@ impl SourceId {
     }
 
     /// Creates an implementation of `Source` corresponding to this ID.
-    pub fn load<'a>(&self, config: &'a Config) -> Box<Source + 'a> {
+    pub fn load<'a>(&self, config: &'a Config) -> CargoResult<Box<Source + 'a>> {
         trace!("loading SourceId; {}", self);
         match self.inner.kind {
-            Kind::Git(..) => Box::new(GitSource::new(self, config)),
+            Kind::Git(..) => Ok(Box::new(GitSource::new(self, config)?)),
             Kind::Path => {
                 let path = match self.inner.url.to_file_path() {
                     Ok(p) => p,
                     Err(()) => panic!("path sources cannot be remote"),
                 };
-                Box::new(PathSource::new(&path, self, config))
+                Ok(Box::new(PathSource::new(&path, self, config)))
             }
-            Kind::Registry => Box::new(RegistrySource::remote(self, config)),
+            Kind::Registry => Ok(Box::new(RegistrySource::remote(self, config))),
             Kind::LocalRegistry => {
                 let path = match self.inner.url.to_file_path() {
                     Ok(p) => p,
                     Err(()) => panic!("path sources cannot be remote"),
                 };
-                Box::new(RegistrySource::local(self, &path, config))
+                Ok(Box::new(RegistrySource::local(self, &path, config)))
             }
             Kind::Directory => {
                 let path = match self.inner.url.to_file_path() {
                     Ok(p) => p,
                     Err(()) => panic!("path sources cannot be remote"),
                 };
-                Box::new(DirectorySource::new(&path, self, config))
+                Ok(Box::new(DirectorySource::new(&path, self, config)))
             }
         }
     }
@@ -575,15 +576,15 @@ mod tests {
     fn github_sources_equal() {
         let loc = "https://github.com/foo/bar".to_url().unwrap();
         let master = Kind::Git(GitReference::Branch("master".to_string()));
-        let s1 = SourceId::new(master.clone(), loc);
+        let s1 = SourceId::new(master.clone(), loc).unwrap();
 
         let loc = "git://github.com/foo/bar".to_url().unwrap();
-        let s2 = SourceId::new(master, loc.clone());
+        let s2 = SourceId::new(master, loc.clone()).unwrap();
 
         assert_eq!(s1, s2);
 
         let foo = Kind::Git(GitReference::Branch("foo".to_string()));
-        let s3 = SourceId::new(foo, loc);
+        let s3 = SourceId::new(foo, loc).unwrap();
         assert!(s1 != s3);
     }
 }

diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs
index 808192999aa986050f11e79027cb1a18b7773588..3ce46881b61fa03efcbc370020246896df246fe1 100644 (file)
--- a/src/cargo/ops/cargo_install.rs
+++ b/src/cargo/ops/cargo_install.rs
@@ -133,7 +133,7 @@ fn install_one(root: Filesystem,
     let config = opts.config;
 
     let (pkg, source) = if source_id.is_git() {
-        select_pkg(GitSource::new(source_id, config),
+        select_pkg(GitSource::new(source_id, config)?,
                    krate, vers, config, is_first_install,
                    &mut |git| git.read_packages())?
     } else if source_id.is_path() {

diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
index ddbf99896b1eb9a74cca2fe99eb741c35ccb0224..f7153c19067f7abfc8ca27bdd5442bedfb91c0e1 100644 (file)
--- a/src/cargo/ops/registry.rs
+++ b/src/cargo/ops/registry.rs
@@ -195,7 +195,7 @@ pub fn registry(config: &Config,
     } = registry_configuration(config)?;
     let token = token.or(token_config);
     let sid = match index {
-        Some(index) => SourceId::for_registry(&index.to_url()?),
+        Some(index) => SourceId::for_registry(&index.to_url()?)?,
         None => SourceId::crates_io(config)?,
     };
     let api_host = {

diff --git a/src/cargo/sources/config.rs b/src/cargo/sources/config.rs
index faae80c2a0d5425bb61e70e6dd6df6286338fa31..12e9c07e8ea8d9385d8dd6e8ca2f80200f6c8511 100644 (file)
--- a/src/cargo/sources/config.rs
+++ b/src/cargo/sources/config.rs
@@ -73,7 +73,7 @@ impl<'cfg> SourceConfigMap<'cfg> {
         debug!("loading: {}", id);
         let mut name = match self.id2name.get(id) {
             Some(name) => name,
-            None => return Ok(id.load(self.config)),
+            None => return Ok(id.load(self.config)?),
         };
         let mut path = Path::new("/");
         let orig_name = name;
@@ -91,7 +91,7 @@ impl<'cfg> SourceConfigMap<'cfg> {
                     name = s;
                     path = p;
                 }
-                None if *id == cfg.id => return Ok(id.load(self.config)),
+                None if *id == cfg.id => return Ok(id.load(self.config)?),
                 None => {
                     new_id = cfg.id.with_precise(id.precise()
                                                  .map(|s| s.to_string()));
@@ -105,8 +105,8 @@ impl<'cfg> SourceConfigMap<'cfg> {
                        (configuration in `{}`)", name, path.display())
             }
         }
-        let new_src = new_id.load(self.config);
-        let old_src = id.load(self.config);
+        let new_src = new_id.load(self.config)?;
+        let old_src = id.load(self.config)?;
         if new_src.supports_checksums() != old_src.supports_checksums() {
             let (supports, no_support) = if new_src.supports_checksums() {
                 (name, orig_name)
@@ -133,7 +133,7 @@ a lock file compatible with `{orig}` cannot be generated in this situation
         let mut srcs = Vec::new();
         if let Some(val) = table.get("registry") {
             let url = url(val, &format!("source.{}.registry", name))?;
-            srcs.push(SourceId::for_registry(&url));
+            srcs.push(SourceId::for_registry(&url)?);
         }
         if let Some(val) = table.get("local-registry") {
             let (s, path) = val.string(&format!("source.{}.local-registry",

diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs
index 75c0281a4d31abf694f9e8e52dd1de79f18fca46..ed156af83b17eaccbb03d54c6a61c8baa184b12d 100644 (file)
--- a/src/cargo/sources/git/source.rs
+++ b/src/cargo/sources/git/source.rs
@@ -25,18 +25,18 @@ pub struct GitSource<'cfg> {
 
 impl<'cfg> GitSource<'cfg> {
     pub fn new(source_id: &SourceId,
-               config: &'cfg Config) -> GitSource<'cfg> {
+               config: &'cfg Config) -> CargoResult<GitSource<'cfg>> {
         assert!(source_id.is_git(), "id is not git, id={}", source_id);
 
         let remote = GitRemote::new(source_id.url());
-        let ident = ident(source_id.url());
+        let ident = ident(source_id.url())?;
 
         let reference = match source_id.precise() {
             Some(s) => GitReference::Rev(s.to_string()),
             None => source_id.git_reference().unwrap().clone(),
         };
 
-        GitSource {
+        let source = GitSource {
             remote: remote,
             reference: reference,
             source_id: source_id.clone(),
@@ -44,7 +44,9 @@ impl<'cfg> GitSource<'cfg> {
             rev: None,
             ident: ident,
             config: config,
-        }
+        };
+
+        Ok(source)
     }
 
     pub fn url(&self) -> &Url { self.remote.url() }
@@ -57,8 +59,8 @@ impl<'cfg> GitSource<'cfg> {
     }
 }
 
-fn ident(url: &Url) -> String {
-    let url = canonicalize_url(url);
+fn ident(url: &Url) -> CargoResult<String> {
+    let url = canonicalize_url(url)?;
     let ident = url.path_segments().and_then(|mut s| s.next_back()).unwrap_or("");
 
     let ident = if ident == "" {
@@ -67,13 +69,22 @@ fn ident(url: &Url) -> String {
         ident
     };
 
-    format!("{}-{}", ident, short_hash(&url))
+    Ok(format!("{}-{}", ident, short_hash(&url)))
 }
 
 // Some hacks and heuristics for making equivalent URLs hash the same
-pub fn canonicalize_url(url: &Url) -> Url {
+pub fn canonicalize_url(url: &Url) -> CargoResult<Url> {
     let mut url = url.clone();
 
+    if url.cannot_be_a_base() {
+        if url.scheme() != "github.com" {
+            return Err(format!("invalid url `{}`: cannot-be-a-base-URLs are not supported", url).into());
+        }
+        // it's most likely an attempt to fetch github repo
+        // https://github.com/rust-lang/cargo/issues/4394
+        url = Url::parse(&format!("https://github.com/{}", url.path())).unwrap();
+    }
+
     // Strip a trailing slash
     if url.path().ends_with('/') {
         url.path_segments_mut().unwrap().pop_if_empty();
@@ -100,7 +111,7 @@ pub fn canonicalize_url(url: &Url) -> Url {
         url.path_segments_mut().unwrap().pop().push(&last);
     }
 
-    url
+    Ok(url)
 }
 
 impl<'cfg> Debug for GitSource<'cfg> {
@@ -202,44 +213,57 @@ mod test {
 
     #[test]
     pub fn test_url_to_path_ident_with_path() {
-        let ident = ident(&url("https://github.com/carlhuda/cargo"));
+        let ident = ident(&url("https://github.com/carlhuda/cargo")).unwrap();
         assert!(ident.starts_with("cargo-"));
     }
 
     #[test]
     pub fn test_url_to_path_ident_without_path() {
-        let ident = ident(&url("https://github.com"));
+        let ident = ident(&url("https://github.com")).unwrap();
         assert!(ident.starts_with("_empty-"));
     }
 
     #[test]
     fn test_canonicalize_idents_by_stripping_trailing_url_slash() {
-        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston/"));
-        let ident2 = ident(&url("https://github.com/PistonDevelopers/piston"));
+        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston/")).unwrap();
+        let ident2 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap();
         assert_eq!(ident1, ident2);
     }
 
     #[test]
     fn test_canonicalize_idents_by_lowercasing_github_urls() {
-        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston"));
-        let ident2 = ident(&url("https://github.com/pistondevelopers/piston"));
+        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap();
+        let ident2 = ident(&url("https://github.com/pistondevelopers/piston")).unwrap();
         assert_eq!(ident1, ident2);
     }
 
     #[test]
     fn test_canonicalize_idents_by_stripping_dot_git() {
-        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston"));
-        let ident2 = ident(&url("https://github.com/PistonDevelopers/piston.git"));
+        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap();
+        let ident2 = ident(&url("https://github.com/PistonDevelopers/piston.git")).unwrap();
         assert_eq!(ident1, ident2);
     }
 
     #[test]
     fn test_canonicalize_idents_different_protocls() {
-        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston"));
-        let ident2 = ident(&url("git://github.com/PistonDevelopers/piston"));
+        let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap();
+        let ident2 = ident(&url("git://github.com/PistonDevelopers/piston")).unwrap();
         assert_eq!(ident1, ident2);
     }
 
+    #[test]
+    fn test_canonicalize_github_not_a_base_urls() {
+        let ident1 = ident(&url("github.com:PistonDevelopers/piston")).unwrap();
+        let ident2 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap();
+        assert_eq!(ident1, ident2);
+    }
+
+    #[test]
+    fn test_canonicalize_not_a_base_urls() {
+        assert!(ident(&url("github.com:PistonDevelopers/piston")).is_ok());
+        assert!(ident(&url("google.com:PistonDevelopers/piston")).is_err());
+    }
+
     fn url(s: &str) -> Url {
         s.to_url().unwrap()
     }

diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs
index fcf5a98d1e850becd386aa2d59d44f0534d91520..3b444d8322c138b55271b3ea0d0080f5e54c314e 100644 (file)
--- a/src/cargo/util/toml/mod.rs
+++ b/src/cargo/util/toml/mod.rs
@@ -876,7 +876,7 @@ impl TomlDependency {
                     .or_else(|| details.rev.clone().map(GitReference::Rev))
                     .unwrap_or_else(|| GitReference::Branch("master".to_string()));
                 let loc = git.to_url()?;
-                SourceId::for_git(&loc, reference)
+                SourceId::for_git(&loc, reference)?
             },
             (None, Some(path)) => {
                 cx.nested_paths.push(PathBuf::from(path));

diff --git a/tests/resolve.rs b/tests/resolve.rs
index 144fb608b2adf07c3da9db2cb891098dfe10fe93..16395a255671e1e08e92975691309de9181adf98 100644 (file)
--- a/tests/resolve.rs
+++ b/tests/resolve.rs
@@ -44,7 +44,7 @@ trait ToDep {
 impl ToDep for &'static str {
     fn to_dep(self) -> Dependency {
         let url = "http://example.com".to_url().unwrap();
-        let source_id = SourceId::for_registry(&url);
+        let source_id = SourceId::for_registry(&url).unwrap();
         Dependency::parse_no_deprecated(self, Some("1.0.0"), &source_id).unwrap()
     }
 }
@@ -86,7 +86,7 @@ macro_rules! pkg {
 
 fn registry_loc() -> SourceId {
     let remote = "http://example.com".to_url().unwrap();
-    SourceId::for_registry(&remote)
+    SourceId::for_registry(&remote).unwrap()
 }
 
 fn pkg(name: &str) -> Summary {
@@ -100,7 +100,7 @@ fn pkg_id(name: &str) -> PackageId {
 fn pkg_id_loc(name: &str, loc: &str) -> PackageId {
     let remote = loc.to_url();
     let master = GitReference::Branch("master".to_string());
-    let source_id = SourceId::for_git(&remote.unwrap(), master);
+    let source_id = SourceId::for_git(&remote.unwrap(), master).unwrap();
 
     PackageId::new(name, "1.0.0", &source_id).unwrap()
 }
@@ -112,14 +112,14 @@ fn pkg_loc(name: &str, loc: &str) -> Summary {
 fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") }
 fn dep_req(name: &str, req: &str) -> Dependency {
     let url = "http://example.com".to_url().unwrap();
-    let source_id = SourceId::for_registry(&url);
+    let source_id = SourceId::for_registry(&url).unwrap();
     Dependency::parse_no_deprecated(name, Some(req), &source_id).unwrap()
 }
 
 fn dep_loc(name: &str, location: &str) -> Dependency {
     let url = location.to_url().unwrap();
     let master = GitReference::Branch("master".to_string());
-    let source_id = SourceId::for_git(&url, master);
+    let source_id = SourceId::for_git(&url, master).unwrap();
     Dependency::parse_no_deprecated(name, Some("1.0.0"), &source_id).unwrap()
 }
 fn dep_kind(name: &str, kind: Kind) -> Dependency {