From c7b9a75c6f8f671c0d76ac7739cd0a7c05453b06 Mon Sep 17 00:00:00 2001 From: Cezary Baginski Date: Thu, 13 Nov 2014 12:55:57 +0100 Subject: [PATCH] abort on symlink loop + refactor Record#build --- lib/listen/record.rb | 54 +++++++-------- lib/listen/record/entry.rb | 51 ++++++++++++++ lib/listen/record/symlink_detector.rb | 59 ++++++++++++++++ spec/lib/listen/record_spec.rb | 98 ++++++++++++++++----------- 4 files changed, 194 insertions(+), 68 deletions(-) create mode 100644 lib/listen/record/entry.rb create mode 100644 lib/listen/record/symlink_detector.rb diff --git a/lib/listen/record.rb b/lib/listen/record.rb index c3ea697d..adb9056d 100644 --- a/lib/listen/record.rb +++ b/lib/listen/record.rb @@ -1,7 +1,9 @@ +require 'listen/record/entry' +require 'listen/record/symlink_detector' + module Listen class Record include Celluloid - # TODO: one Record object per watched directory? # TODO: deprecate @@ -100,34 +102,32 @@ def _fast_unset_path(dir, dirname, basename) end end + # TODO: test with a file name given + # TODO: test other permissions + # TODO: test with mixed encoding def _fast_build(root) + symlink_detector = SymlinkDetector.new @paths[root] = _auto_hash - left = Queue.new - left << '.' - - until left.empty? - dirname = left.pop - add_dir(root, dirname) - - path = ::File.join(root, dirname) - current = Dir.entries(path.to_s) - %w(. ..) - - current.each do |entry| - full_path = ::File.join(path, entry) - - if Dir.exist?(full_path) - left << (dirname == '.' ? entry : ::File.join(dirname, entry)) - else - begin - lstat = ::File.lstat(full_path) - data = { mtime: lstat.mtime.to_f, mode: lstat.mode } - _fast_update_file(root, dirname, entry, data) - rescue SystemCallError - _fast_unset_path(root, dirname, entry) - end - end - end - end + remaining = Queue.new + remaining << Entry.new(root, nil, nil) + _fast_build_dir(remaining, symlink_detector) until remaining.empty? + end + + def _fast_build_dir(remaining, symlink_detector) + entry = remaining.pop + entry.children.each { |child| remaining << child } + symlink_detector.verify_unwatched!(entry) + add_dir(entry.root, entry.record_dir_key) + rescue Errno::ENOTDIR + _fast_try_file(entry) + rescue SystemCallError + _fast_unset_path(entry.root, entry.relative, entry.name) + end + + def _fast_try_file(entry) + _fast_update_file(entry.root, entry.relative, entry.name, entry.meta) + rescue SystemCallError + _fast_unset_path(entry.root, entry.relative, entry.name) end end end diff --git a/lib/listen/record/entry.rb b/lib/listen/record/entry.rb new file mode 100644 index 00000000..1080977c --- /dev/null +++ b/lib/listen/record/entry.rb @@ -0,0 +1,51 @@ +module Listen + # @private api + class Record + # Represents a directory entry (dir or file) + class Entry + # file: "/home/me/watched_dir", "app/models", "foo.rb" + # dir, "/home/me/watched_dir", "." + def initialize(root, relative, name = nil) + @root, @relative, @name = root, relative, name + end + + attr_reader :root, :relative, :name + + def children + child_relative = _join + (Dir.entries(sys_path) - %w(. ..)).map do |name| + Entry.new(@root, child_relative, name) + end + end + + def meta + lstat = ::File.lstat(sys_path) + { mtime: lstat.mtime.to_f, mode: lstat.mode } + end + + # record hash is e.g. + # if @record["/home/me/watched_dir"]["project/app/models"]["foo.rb"] + # if @record["/home/me/watched_dir"]["project/app"]["models"] + # record_dir_key is "project/app/models" + def record_dir_key + ::File.join(*[@relative, @name].compact) + end + + def sys_path + # Use full path in case someone uses chdir + ::File.join(*[@root, @relative, @name].compact) + end + + def real_path + @real_path ||= ::File.realpath(sys_path) + end + + private + + def _join + args = [@relative, @name].compact + args.empty? ? nil : ::File.join(*args) + end + end + end +end diff --git a/lib/listen/record/symlink_detector.rb b/lib/listen/record/symlink_detector.rb new file mode 100644 index 00000000..910f8b56 --- /dev/null +++ b/lib/listen/record/symlink_detector.rb @@ -0,0 +1,59 @@ +require 'set' + +module Listen + # @private api + class Record + class SymlinkDetector + SYMLINK_LOOP_ERROR = <<-EOS + ** ERROR: Listen detected a duplicate directory being watched! ** + + (This may be due to symlinks pointing to parent directories). + + Duplicate: %s + + which already is added as: %s + + Listen is refusing to continue, because this may likely result in + an infinite loop. + + Suggestions: + + 1) (best option) watch only directories you care about, e.g. + either symlinked folders or folders with the real directories, + but not both. + + 2) reorganize your project so that watched directories do not + contain symlinked directories + + 3) submit patches so that Listen can reliably and quickly (!) + detect symlinks to already watched read directories, skip + them, and then reasonably choose which symlinked paths to + report as changed (if any) + + 4) (not worth it) help implement a "reverse symlink lookup" + function in Listen, which - given a real directory - would + return all the symlinks pointing to that directory + + Issue: https://github.com/guard/listen/issues/259 + EOS + + def initialize + @real_dirs = Set.new + end + + def verify_unwatched!(entry) + real_path = entry.real_path + @real_dirs.add?(real_path) || _fail(entry.sys_path, real_path) + end + + private + + def _fail(symlinked, real_path) + STDERR.puts format(SYMLINK_LOOP_ERROR, symlinked, real_path) + + # Note Celluloid eats up abort message anyway + fail 'Failed due to looped symlinks' + end + end + end +end diff --git a/spec/lib/listen/record_spec.rb b/spec/lib/listen/record_spec.rb index a2126721..937dfab2 100644 --- a/spec/lib/listen/record_spec.rb +++ b/spec/lib/listen/record_spec.rb @@ -193,21 +193,24 @@ before do allow(listener).to receive(:directories) { directories } - allow(::File).to receive(:lstat) do |path| - fail "::File.lstat stub called with: #{path.inspect}" - end - - allow(::Dir).to receive(:entries) do |path| - fail "::Dir.entries stub called with: #{path.inspect}" - end - - allow(::Dir).to receive(:exist?) do |path| - fail "::Dir.exist? stub called with: #{path.inspect}" + stubs = { + ::File => %w(lstat realpath), + ::Dir => %w(entries exist?) + } + + stubs.each do |klass, meths| + meths.each do |meth| + allow(klass).to receive(meth.to_sym) do |*args| + fail "stub called: #{klass}.#{meth}(#{args.map(&:inspect) * ', '})" + end + end end end it 're-inits paths' do - allow(::Dir).to receive(:entries) { [] } + allow(::Dir).to receive(:entries).and_return([]) + allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1') + allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2') record.update_file(dir, 'path/file.rb', mtime: 1.1) record.build @@ -219,18 +222,19 @@ let(:bar_stat) { instance_double(::File::Stat, mtime: 2.3, mode: 0755) } context 'with no subdirs' do - before do - expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo bar) } - expect(::Dir).to receive(:exist?).with('/dir1/./foo') { false } - expect(::Dir).to receive(:exist?).with('/dir1/./bar') { false } - expect(::File).to receive(:lstat).with('/dir1/./foo') { foo_stat } - expect(::File).to receive(:lstat).with('/dir1/./bar') { bar_stat } - - expect(::Dir).to receive(:entries).with('/dir2/.') { [] } + allow(::Dir).to receive(:entries).with('/dir1') { %w(foo bar) } + allow(::Dir).to receive(:entries).with('/dir1/foo').and_raise(Errno::ENOTDIR) + allow(::Dir).to receive(:entries).with('/dir1/bar').and_raise(Errno::ENOTDIR) + allow(::File).to receive(:lstat).with('/dir1/foo') { foo_stat } + allow(::File).to receive(:lstat).with('/dir1/bar') { bar_stat } + allow(::Dir).to receive(:entries).with('/dir2') { [] } + + allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1') + allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2') end - it 'builds record' do + it 'builds record' do record.build expect(record.paths.keys).to eq %w( /dir1 /dir2 ) expect(record.paths['/dir1']). @@ -242,15 +246,16 @@ context 'with subdir containing files' do before do - expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo) } - expect(::Dir).to receive(:exist?).with('/dir1/./foo') { true } - - expect(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar) } - - expect(::Dir).to receive(:exist?).with('/dir1/foo/bar') { false } - expect(::File).to receive(:lstat).with('/dir1/foo/bar') { bar_stat } - - expect(::Dir).to receive(:entries).with('/dir2/.') { [] } + allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) } + allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar) } + allow(::Dir).to receive(:entries).with('/dir1/foo/bar').and_raise(Errno::ENOTDIR) + allow(::File).to receive(:lstat).with('/dir1/foo/bar') { bar_stat } + allow(::Dir).to receive(:entries).with('/dir2') { [] } + + allow(::File).to receive(:realpath).with('/dir1').and_return('/dir1') + allow(::File).to receive(:realpath).with('/dir2').and_return('/dir2') + allow(::File).to receive(:realpath).with('/dir1/foo'). + and_return('/dir1/foo') end it 'builds record' do @@ -264,18 +269,12 @@ context 'with subdir containing dirs' do before do - expect(::Dir).to receive(:entries).with('/dir1/.') { %w(foo) } - expect(::Dir).to receive(:exist?).with('/dir1/./foo') { true } - - expect(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar baz) } - - expect(::Dir).to receive(:exist?).with('/dir1/foo/bar') { true } - expect(::Dir).to receive(:entries).with('/dir1/foo/bar') { [] } - - expect(::Dir).to receive(:exist?).with('/dir1/foo/baz') { true } - expect(::Dir).to receive(:entries).with('/dir1/foo/baz') { [] } - - expect(::Dir).to receive(:entries).with('/dir2/.') { [] } + allow(::File).to receive(:realpath) { |path| path } + allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) } + allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(bar baz) } + allow(::Dir).to receive(:entries).with('/dir1/foo/bar') { [] } + allow(::Dir).to receive(:entries).with('/dir1/foo/baz') { [] } + allow(::Dir).to receive(:entries).with('/dir2') { [] } end it 'builds record' do @@ -290,5 +289,22 @@ expect(record.paths['/dir2']).to eq({}) end end + + context 'with subdir containing symlink to parent' do + subject { record.paths } + before do + allow(::Dir).to receive(:entries).with('/dir1') { %w(foo) } + allow(::Dir).to receive(:entries).with('/dir1/foo') { %w(foo) } + allow(::File).to receive(:realpath).with('/dir1').and_return('/bar') + allow(::File).to receive(:realpath).with('/dir1/foo').and_return('/bar') + end + + it 'shows message and aborts with error' do + expect(STDERR).to receive(:puts).with(/detected a duplicate directory/) + + expect { record.build }.to raise_error(RuntimeError, + /Failed due to looped symlinks/) + end + end end end