Skip to content

Commit 653e688

Browse files
committed
auto merge of #21113 : alexcrichton/rust/plug-a-hole, r=brson
With the addition of separate search paths to the compiler, it was intended that applications such as Cargo could require a `--extern` flag per `extern crate` directive in the source. The system can currently be subverted, however, due to the `existing_match()` logic in the crate loader. When loading crates we first attempt to match an `extern crate` directive against all previously loaded crates to avoid reading metadata twice. This "hit the cache if possible" step was erroneously leaking crates across the search path boundaries, however. For example: extern crate b; extern crate a; If `b` depends on `a`, then it will load crate `a` when the `extern crate b` directive is being processed. When the compiler reaches `extern crate a` it will use the previously loaded version no matter what. If the compiler was not invoked with `-L crate=path/to/a`, it will still succeed. This behavior is allowing `extern crate` declarations in Cargo without a corresponding declaration in the manifest of a dependency, which is considered a bug. This commit fixes this problem by keeping track of the origin search path for a crate. Crates loaded from the dependency search path are not candidates for crates which are loaded from the crate search path.
2 parents 210f0dc + cbeb77e commit 653e688

File tree

15 files changed

+145
-78
lines changed

15 files changed

+145
-78
lines changed

src/librustc/metadata/creader.rs

+38-26
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ fn dump_crates(cstore: &CStore) {
5858
debug!(" hash: {}", data.hash());
5959
opt_source.map(|cs| {
6060
let CrateSource { dylib, rlib, cnum: _ } = cs;
61-
dylib.map(|dl| debug!(" dylib: {}", dl.display()));
62-
rlib.map(|rl| debug!(" rlib: {}", rl.display()));
61+
dylib.map(|dl| debug!(" dylib: {}", dl.0.display()));
62+
rlib.map(|rl| debug!(" rlib: {}", rl.0.display()));
6363
});
6464
})
6565
}
@@ -305,8 +305,8 @@ impl<'a> CrateReader<'a> {
305305
}
306306
}
307307

308-
fn existing_match(&self, name: &str,
309-
hash: Option<&Svh>) -> Option<ast::CrateNum> {
308+
fn existing_match(&self, name: &str, hash: Option<&Svh>, kind: PathKind)
309+
-> Option<ast::CrateNum> {
310310
let mut ret = None;
311311
self.sess.cstore.iter_crate_data(|cnum, data| {
312312
if data.name != name { return }
@@ -317,27 +317,37 @@ impl<'a> CrateReader<'a> {
317317
None => {}
318318
}
319319

320-
// When the hash is None we're dealing with a top-level dependency in
321-
// which case we may have a specification on the command line for this
322-
// library. Even though an upstream library may have loaded something of
323-
// the same name, we have to make sure it was loaded from the exact same
324-
// location as well.
320+
// When the hash is None we're dealing with a top-level dependency
321+
// in which case we may have a specification on the command line for
322+
// this library. Even though an upstream library may have loaded
323+
// something of the same name, we have to make sure it was loaded
324+
// from the exact same location as well.
325325
//
326326
// We're also sure to compare *paths*, not actual byte slices. The
327327
// `source` stores paths which are normalized which may be different
328328
// from the strings on the command line.
329329
let source = self.sess.cstore.get_used_crate_source(cnum).unwrap();
330-
match self.sess.opts.externs.get(name) {
331-
Some(locs) => {
332-
let found = locs.iter().any(|l| {
333-
let l = fs::realpath(&Path::new(&l[])).ok();
334-
l == source.dylib || l == source.rlib
335-
});
336-
if found {
337-
ret = Some(cnum);
338-
}
330+
if let Some(locs) = self.sess.opts.externs.get(name) {
331+
let found = locs.iter().any(|l| {
332+
let l = fs::realpath(&Path::new(&l[])).ok();
333+
source.dylib.as_ref().map(|p| &p.0) == l.as_ref() ||
334+
source.rlib.as_ref().map(|p| &p.0) == l.as_ref()
335+
});
336+
if found {
337+
ret = Some(cnum);
339338
}
340-
None => ret = Some(cnum),
339+
}
340+
341+
// Alright, so we've gotten this far which means that `data` has the
342+
// right name, we don't have a hash, and we don't have a --extern
343+
// pointing for ourselves. We're still not quite yet done because we
344+
// have to make sure that this crate was found in the crate lookup
345+
// path (this is a top-level dependency) as we don't want to
346+
// implicitly load anything inside the dependency lookup path.
347+
let prev_kind = source.dylib.as_ref().or(source.rlib.as_ref())
348+
.unwrap().1;
349+
if ret.is_none() && (prev_kind == kind || prev_kind == PathKind::All) {
350+
ret = Some(cnum);
341351
}
342352
});
343353
return ret;
@@ -359,8 +369,8 @@ impl<'a> CrateReader<'a> {
359369
let crate_paths = if root.is_none() {
360370
Some(CratePaths {
361371
ident: ident.to_string(),
362-
dylib: lib.dylib.clone(),
363-
rlib: lib.rlib.clone(),
372+
dylib: lib.dylib.clone().map(|p| p.0),
373+
rlib: lib.rlib.clone().map(|p| p.0),
364374
})
365375
} else {
366376
None
@@ -400,7 +410,7 @@ impl<'a> CrateReader<'a> {
400410
kind: PathKind)
401411
-> (ast::CrateNum, Rc<cstore::crate_metadata>,
402412
cstore::CrateSource) {
403-
match self.existing_match(name, hash) {
413+
match self.existing_match(name, hash, kind) {
404414
None => {
405415
let mut load_ctxt = loader::Context {
406416
sess: self.sess,
@@ -483,8 +493,8 @@ impl<'a> CrateReader<'a> {
483493
let library = match load_ctxt.maybe_load_library_crate() {
484494
Some(l) => l,
485495
None if is_cross => {
486-
// Try loading from target crates. This will abort later if we try to
487-
// load a plugin registrar function,
496+
// Try loading from target crates. This will abort later if we
497+
// try to load a plugin registrar function,
488498
target_only = true;
489499
should_link = info.should_link;
490500

@@ -497,7 +507,9 @@ impl<'a> CrateReader<'a> {
497507
};
498508

499509
let dylib = library.dylib.clone();
500-
let register = should_link && self.existing_match(info.name.as_slice(), None).is_none();
510+
let register = should_link && self.existing_match(info.name.as_slice(),
511+
None,
512+
PathKind::Crate).is_none();
501513
let metadata = if register {
502514
// Register crate now to avoid double-reading metadata
503515
let (_, cmd, _) = self.register_crate(&None, &info.ident[],
@@ -511,7 +523,7 @@ impl<'a> CrateReader<'a> {
511523
PluginMetadata {
512524
sess: self.sess,
513525
metadata: metadata,
514-
dylib: dylib,
526+
dylib: dylib.map(|p| p.0),
515527
info: info,
516528
vi_span: span,
517529
target_only: target_only,

src/librustc/metadata/cstore.rs

+6-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ pub use self::NativeLibraryKind::*;
2020
use back::svh::Svh;
2121
use metadata::decoder;
2222
use metadata::loader;
23+
use session::search_paths::PathKind;
2324
use util::nodemap::{FnvHashMap, NodeMap};
2425

2526
use std::cell::RefCell;
@@ -65,8 +66,8 @@ pub enum NativeLibraryKind {
6566
// must be non-None.
6667
#[derive(PartialEq, Clone)]
6768
pub struct CrateSource {
68-
pub dylib: Option<Path>,
69-
pub rlib: Option<Path>,
69+
pub dylib: Option<(Path, PathKind)>,
70+
pub rlib: Option<(Path, PathKind)>,
7071
pub cnum: ast::CrateNum,
7172
}
7273

@@ -178,10 +179,10 @@ impl CStore {
178179
let mut libs = self.used_crate_sources.borrow()
179180
.iter()
180181
.map(|src| (src.cnum, match prefer {
181-
RequireDynamic => src.dylib.clone(),
182-
RequireStatic => src.rlib.clone(),
182+
RequireDynamic => src.dylib.clone().map(|p| p.0),
183+
RequireStatic => src.rlib.clone().map(|p| p.0),
183184
}))
184-
.collect::<Vec<(ast::CrateNum, Option<Path>)>>();
185+
.collect::<Vec<_>>();
185186
libs.sort_by(|&(a, _), &(b, _)| {
186187
ordering.position_elem(&a).cmp(&ordering.position_elem(&b))
187188
});

src/librustc/metadata/filesearch.rs

+11-9
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,13 @@ pub struct FileSearch<'a> {
3939

4040
impl<'a> FileSearch<'a> {
4141
pub fn for_each_lib_search_path<F>(&self, mut f: F) where
42-
F: FnMut(&Path) -> FileMatch,
42+
F: FnMut(&Path, PathKind) -> FileMatch,
4343
{
4444
let mut visited_dirs = HashSet::new();
4545
let mut found = false;
4646

47-
for path in self.search_paths.iter(self.kind) {
48-
match f(path) {
47+
for (path, kind) in self.search_paths.iter(self.kind) {
48+
match f(path, kind) {
4949
FileMatches => found = true,
5050
FileDoesntMatch => ()
5151
}
@@ -56,7 +56,7 @@ impl<'a> FileSearch<'a> {
5656
let tlib_path = make_target_lib_path(self.sysroot,
5757
self.triple);
5858
if !visited_dirs.contains(tlib_path.as_vec()) {
59-
match f(&tlib_path) {
59+
match f(&tlib_path, PathKind::All) {
6060
FileMatches => found = true,
6161
FileDoesntMatch => ()
6262
}
@@ -76,7 +76,7 @@ impl<'a> FileSearch<'a> {
7676
visited_dirs.insert(tlib_path.as_vec().to_vec());
7777
// Don't keep searching the RUST_PATH if one match turns up --
7878
// if we did, we'd get a "multiple matching crates" error
79-
match f(&tlib_path) {
79+
match f(&tlib_path, PathKind::All) {
8080
FileMatches => {
8181
break;
8282
}
@@ -91,8 +91,10 @@ impl<'a> FileSearch<'a> {
9191
make_target_lib_path(self.sysroot, self.triple)
9292
}
9393

94-
pub fn search<F>(&self, mut pick: F) where F: FnMut(&Path) -> FileMatch {
95-
self.for_each_lib_search_path(|lib_search_path| {
94+
pub fn search<F>(&self, mut pick: F)
95+
where F: FnMut(&Path, PathKind) -> FileMatch
96+
{
97+
self.for_each_lib_search_path(|lib_search_path, kind| {
9698
debug!("searching {}", lib_search_path.display());
9799
match fs::readdir(lib_search_path) {
98100
Ok(files) => {
@@ -108,7 +110,7 @@ impl<'a> FileSearch<'a> {
108110
let files2 = files.iter().filter(|p| !is_rlib(p));
109111
for path in files1.chain(files2) {
110112
debug!("testing {}", path.display());
111-
let maybe_picked = pick(path);
113+
let maybe_picked = pick(path, kind);
112114
match maybe_picked {
113115
FileMatches => {
114116
debug!("picked {}", path.display());
@@ -142,7 +144,7 @@ impl<'a> FileSearch<'a> {
142144
// Returns a list of directories where target-specific dylibs might be located.
143145
pub fn get_dylib_search_paths(&self) -> Vec<Path> {
144146
let mut paths = Vec::new();
145-
self.for_each_lib_search_path(|lib_search_path| {
147+
self.for_each_lib_search_path(|lib_search_path, _| {
146148
paths.push(lib_search_path.clone());
147149
FileDoesntMatch
148150
});

src/librustc/metadata/loader.rs

+25-22
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@
215215
use back::archive::{METADATA_FILENAME};
216216
use back::svh::Svh;
217217
use session::Session;
218+
use session::search_paths::PathKind;
218219
use llvm;
219220
use llvm::{False, ObjectFile, mk_section_iter};
220221
use llvm::archive_ro::ArchiveRO;
@@ -229,7 +230,7 @@ use rustc_back::target::Target;
229230

230231
use std::ffi::CString;
231232
use std::cmp;
232-
use std::collections::{HashMap, HashSet};
233+
use std::collections::HashMap;
233234
use std::io::fs::PathExtensions;
234235
use std::io;
235236
use std::ptr;
@@ -260,8 +261,8 @@ pub struct Context<'a> {
260261
}
261262

262263
pub struct Library {
263-
pub dylib: Option<Path>,
264-
pub rlib: Option<Path>,
264+
pub dylib: Option<(Path, PathKind)>,
265+
pub rlib: Option<(Path, PathKind)>,
265266
pub metadata: MetadataBlob,
266267
}
267268

@@ -384,7 +385,7 @@ impl<'a> Context<'a> {
384385
// of the crate id (path/name/id).
385386
//
386387
// The goal of this step is to look at as little metadata as possible.
387-
self.filesearch.search(|path| {
388+
self.filesearch.search(|path, kind| {
388389
let file = match path.filename_str() {
389390
None => return FileDoesntMatch,
390391
Some(file) => file,
@@ -404,12 +405,12 @@ impl<'a> Context<'a> {
404405

405406
let hash_str = hash.to_string();
406407
let slot = candidates.entry(hash_str).get().unwrap_or_else(
407-
|vacant_entry| vacant_entry.insert((HashSet::new(), HashSet::new())));
408+
|vacant_entry| vacant_entry.insert((HashMap::new(), HashMap::new())));
408409
let (ref mut rlibs, ref mut dylibs) = *slot;
409410
if rlib {
410-
rlibs.insert(fs::realpath(path).unwrap());
411+
rlibs.insert(fs::realpath(path).unwrap(), kind);
411412
} else {
412-
dylibs.insert(fs::realpath(path).unwrap());
413+
dylibs.insert(fs::realpath(path).unwrap(), kind);
413414
}
414415

415416
FileMatches
@@ -453,16 +454,16 @@ impl<'a> Context<'a> {
453454
self.sess.note("candidates:");
454455
for lib in libraries.iter() {
455456
match lib.dylib {
456-
Some(ref p) => {
457+
Some((ref p, _)) => {
457458
self.sess.note(&format!("path: {}",
458459
p.display())[]);
459460
}
460461
None => {}
461462
}
462463
match lib.rlib {
463-
Some(ref p) => {
464+
Some((ref p, _)) => {
464465
self.sess.note(&format!("path: {}",
465-
p.display())[]);
466+
p.display())[]);
466467
}
467468
None => {}
468469
}
@@ -483,9 +484,9 @@ impl<'a> Context<'a> {
483484
// read the metadata from it if `*slot` is `None`. If the metadata couldn't
484485
// be read, it is assumed that the file isn't a valid rust library (no
485486
// errors are emitted).
486-
fn extract_one(&mut self, m: HashSet<Path>, flavor: &str,
487-
slot: &mut Option<MetadataBlob>) -> Option<Path> {
488-
let mut ret = None::<Path>;
487+
fn extract_one(&mut self, m: HashMap<Path, PathKind>, flavor: &str,
488+
slot: &mut Option<MetadataBlob>) -> Option<(Path, PathKind)> {
489+
let mut ret = None::<(Path, PathKind)>;
489490
let mut error = 0u;
490491

491492
if slot.is_some() {
@@ -500,7 +501,7 @@ impl<'a> Context<'a> {
500501
}
501502
}
502503

503-
for lib in m.into_iter() {
504+
for (lib, kind) in m.into_iter() {
504505
info!("{} reading metadata from: {}", flavor, lib.display());
505506
let metadata = match get_metadata_section(self.target.options.is_like_osx,
506507
&lib) {
@@ -525,7 +526,7 @@ impl<'a> Context<'a> {
525526
self.crate_name)[]);
526527
self.sess.span_note(self.span,
527528
&format!(r"candidate #1: {}",
528-
ret.as_ref().unwrap()
529+
ret.as_ref().unwrap().0
529530
.display())[]);
530531
error = 1;
531532
ret = None;
@@ -538,7 +539,7 @@ impl<'a> Context<'a> {
538539
continue
539540
}
540541
*slot = Some(metadata);
541-
ret = Some(lib);
542+
ret = Some((lib, kind));
542543
}
543544
return if error > 0 {None} else {ret}
544545
}
@@ -606,8 +607,8 @@ impl<'a> Context<'a> {
606607
// rlibs/dylibs.
607608
let sess = self.sess;
608609
let dylibname = self.dylibname();
609-
let mut rlibs = HashSet::new();
610-
let mut dylibs = HashSet::new();
610+
let mut rlibs = HashMap::new();
611+
let mut dylibs = HashMap::new();
611612
{
612613
let mut locs = locs.iter().map(|l| Path::new(&l[])).filter(|loc| {
613614
if !loc.exists() {
@@ -637,13 +638,15 @@ impl<'a> Context<'a> {
637638
false
638639
});
639640

640-
// Now that we have an iterator of good candidates, make sure there's at
641-
// most one rlib and at most one dylib.
641+
// Now that we have an iterator of good candidates, make sure
642+
// there's at most one rlib and at most one dylib.
642643
for loc in locs {
643644
if loc.filename_str().unwrap().ends_with(".rlib") {
644-
rlibs.insert(fs::realpath(&loc).unwrap());
645+
rlibs.insert(fs::realpath(&loc).unwrap(),
646+
PathKind::ExternFlag);
645647
} else {
646-
dylibs.insert(fs::realpath(&loc).unwrap());
648+
dylibs.insert(fs::realpath(&loc).unwrap(),
649+
PathKind::ExternFlag);
647650
}
648651
}
649652
};

src/librustc/session/search_paths.rs

+6-3
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ pub enum PathKind {
2525
Native,
2626
Crate,
2727
Dependency,
28+
ExternFlag,
2829
All,
2930
}
3031

@@ -54,14 +55,16 @@ impl SearchPaths {
5455
}
5556

5657
impl<'a> Iterator for Iter<'a> {
57-
type Item = &'a Path;
58+
type Item = (&'a Path, PathKind);
5859

59-
fn next(&mut self) -> Option<&'a Path> {
60+
fn next(&mut self) -> Option<(&'a Path, PathKind)> {
6061
loop {
6162
match self.iter.next() {
6263
Some(&(kind, ref p)) if self.kind == PathKind::All ||
6364
kind == PathKind::All ||
64-
kind == self.kind => return Some(p),
65+
kind == self.kind => {
66+
return Some((p, kind))
67+
}
6568
Some(..) => {}
6669
None => return None,
6770
}

0 commit comments

Comments
 (0)