Merge lp:~facundo/ubuntuone-client/lr-stop-nonutf8 into lp:ubuntuone-client

[Facundo Batista]

Ignore non utf8 paths in LR (LP: #696901).

Ignore non utf8 paths in LR.

The one that should check if the path is valid is LocalRescan. It can't be done in the signals processors (that are close to real validation, in the freeze_commit stage).

The reason for this is that if LR finds an invalid path it needs to ignore it; in files there is no problem, but for directories it shouldn't scan that directory later.

So, I added a function in LR, very similar to the one in in platform/linux/filesystem_notification.py; didn't use that because this is more platform independant, will all go away when put everything in unicode, and most importantly because don't want to send invalid notifications and don't write in the invalid log for *every* local rescan.

Tests included, and also added logging when ignoring the path because of symlink.

[Natalia Bidart (nataliabidart)]

Looks great!

review: Approve

[Facundo Batista (facundo)]

Approving with one review.

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~facundo/ubuntuone-client/lr-stop-nonutf8 into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
  testing autoconf2.50... not found.
  testing autoconf... found 2.67
checking for automake >= 1.10...
  testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
  testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
  testing intltoolize... found 0.41.1
checking for pkg-config >= 0.14.0...
  testing pkg-config... found 0.25
checking for gtk-doc >= 1.0...
  testing gtkdocize... found 1.17
Checking for required M4 macros...
Checking for forbidden M4 macros...
Processing ./configure.ac
Running libtoolize...
libtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
Running gtkdocize...
Running aclocal-1.11...
Running autoconf...
Running autoheader...
Running automake-1.11...
Running ./configure --enable-gtk-doc --enable-debug ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... no
checking for mawk... mawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking for /usr/bin/ld option to reload object files... -r
checking for objdump... objdump
checking how to ...

=== modified file 'tests/syncdaemon/test_localrescan.py'
--- tests/syncdaemon/test_localrescan.py	2011-08-12 15:26:13 +0000
+++ tests/syncdaemon/test_localrescan.py	2011-08-22 15:52:23 +0000
@@ -1,7 +1,7 @@
 # Author: Facundo Batista <[e-mail]>
 # Author: Natalia Bidart <[e-mail]>
 #
-# Copyright 2009 Canonical Ltd.
+# Copyright 2009-2011 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -617,6 +617,37 @@
 
         yield self.lr.start()
         self.assertEqual(self.eq.pushed, [])
+        self.assertTrue(self.handler.check_info("Ignoring path",
+                                                "symlink", symlpath))
+
+    @skipIfOS('win32', "Windows paths are already unicode, "
+                       "can't make this explode there")
+    @defer.inlineCallbacks
+    def test_disc_non_utf8_file(self):
+        """Test having a utf8 file."""
+        # create a broken path
+        path = os.path.join(self.share.path, "non utf8 \xff path")
+        with open_file(path, "w") as fh:
+            fh.write("broken")
+
+        yield self.lr.start()
+        self.assertEqual(self.eq.pushed, [])
+        self.assertTrue(self.handler.check_info("Ignoring path",
+                                                "invalid (non utf8)"))
+
+    @skipIfOS('win32', "Windows paths are already unicode, "
+                       "can't make this explode there")
+    @defer.inlineCallbacks
+    def test_disc_non_utf8_dir(self):
+        """Test having a utf8 dir."""
+        # create a broken path
+        path = os.path.join(self.share.path, "non utf8 \xff path")
+        make_dir(path)
+
+        yield self.lr.start()
+        self.assertEqual(self.eq.pushed, [])
+        self.assertTrue(self.handler.check_info("Ignoring path",
+                                                "invalid (non utf8)"))
 
     @defer.inlineCallbacks
     def test_disc_more_dir_normal(self):
 
=== modified file 'ubuntuone/syncdaemon/local_rescan.py'
--- ubuntuone/syncdaemon/local_rescan.py	2011-08-10 15:03:17 +0000
+++ ubuntuone/syncdaemon/local_rescan.py	2011-08-22 15:52:23 +0000
@@ -2,7 +2,7 @@
 #
 # Author: Facundo Batista <[e-mail]>
 #
-# Copyright 2009 Canonical Ltd.
+# Copyright 2009-2011 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -56,6 +56,26 @@
 log_warning = functools.partial(lr_logger.log, logging.WARNING)
 
 
+def is_valid_name(path):
+    """Tell if the name is valid.
+
+    This checks for the bytes in the path to be UTF8 valid before let them
+    get into Syncdaemon.
+
+    There is a similar check in platform/linux/filesystem_notification.py; we
+    don't use that because this is more platform independant, will all go away
+    when put everything in unicode, and most importantly because don't want to
+    send invalid notifications and don't write in the invalid log for *every*
+    local rescan.
+    """
+    try:
+        path.decode("utf8")
+    except UnicodeDecodeError:
+        return False
+    else:
+        return True
+
+
 class LocalRescan(object):
     """Local re-scanner.
 
@@ -644,13 +664,22 @@
             for something in dircontent:
                 fullname = os.path.join(dirpath, something)
                 stat_result = get_stat(fullname)
-                if stat_result is None or is_link(fullname):
-                    continue
-                elif not access(fullname):
+                if stat_result is None:
+                    # gone between the listdir and now
+                    continue
+                if is_link(fullname):
+                    log_info("Ignoring path as it's a symlink: %r", fullname)
+                    continue
+                if not is_valid_name(fullname):
+                    m = "Ignoring path because it's invalid (non utf8): %r"
+                    log_info(m, fullname)
+                    continue
+                if not access(fullname):
                     log_warning("Ignoring path as we don't have enough "
                                 "permissions to track it: %r", fullname)
                     continue
-                elif stat.S_ISDIR(stat_result.st_mode):
+
+                if stat.S_ISDIR(stat_result.st_mode):
                     dnames.append(something)
                 elif stat.S_ISREG(stat_result.st_mode):
                     fnames.append(something)