918392 - FileCopier does not handle symlinked directories properly

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Ted was testing test manifests in bug 901990 and encountered an issue with infinite recursion of symlinks:

 stat: /build/mozilla-central/security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp: Too many levels of symbolic links

He claimed the _tests install manifest processing created a recursive symlink. I believe him.

If you have a current style _tests directory sitting around, nsinstall and friends may symlink entire directories under _tests.

e.g.

  security/manager/ssl/tests/unit/xpcshell.ini
  security/manager/ssl/tests/unit/tlsserver/cert8.db

What happens currently inside _tests is:

  security/manager/ssl/tests/unit/xpcshell.ini -> .../unit/xpcshell.ini
  security/manager/ssl/tests/unit/tlsserver -> .../unit/tlsserver

It symlinks the "tlsserver" directory.

When FileCopier comes around, it manages files, not directories. So, it sees tlsserver/cert8.db and is like "cert8.db is a file. But I think it should be a symlink. So, let me remove the file and create a symlink in its place." And it does. You end up with (in the source dir):

  security/manager/ssl/tests/unit/tlsserver/cert8.db -> security/manager/ssl/tests/unit/tlsserver/cert8.db

A symlink pointing to self!

There are a few improvements that can be made here:

1) Make FileCopier aware of directories that are symlinks. Since it can't speak directories (only files), FileCopier should remove symlinks to directories.
2) Have symlinked files check they don't create circular references.

Comment 1

[(not currently active) Ted Mielczarek]

gps' workaround was to rm -rf $objdir/_tests, and rebuilding after that built cleanly.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Handle symlinked directories properly

This appears to work.

I pretty much rewrote the entire function body. In hindsight, I'm not
sure if this is strictly necessary. I probably could have made the diff
smaller at the expense of more system calls. But, I don't like abusing
system calls (even if they may be cached), so I went ahead and optimized
things so we only incur one os.walk().

While I was here, I noticed that the symlink tests weren't actually
running because the setUp() function wasn't setting the flag indicating
symlinks work! It's not clear because I moved that whole function into
another class, but I added this critical line then fixed a symlink test
(the test, not the code, was bad, fortunately).

https://tbpl.mozilla.org/?tree=Try&rev=5d4111cebd5e

Comment 3

[Gregory Szorc [:gps]]

Apparently I broke clobber builds due to my use of mkdir() instead of makedirs() because it appears $(DIST) isn't created as part of the build. Derp.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Handle symlinked directories properly

I hackily inserted a mkdir(dist) in recursivemake.py. I feel dirty.

https://tbpl.mozilla.org/?tree=Try&rev=26b436b29780

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 808089 [details] [diff] [review]
Handle symlinked directories properly

Review of attachment 808089 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +615,5 @@
>              self.summary.managed_count += 1
>  
>          self._write_manifests('install', self._install_manifests)
>  
> +        ensureParentDir(os.path.join(self.environment.topobjdir, 'dist', 'foo'))

maybe add an ensureDir to util?

::: python/mozbuild/mozpack/copier.py
@@ -29,5 @@
> -
> -    if not os.access(dir, os.W_OK):
> -        umask = os.umask(0077)
> -        os.umask(umask)
> -        os.chmod(dir, 0777 & ~umask)

That access thing probably needs to be added to util's ensureParentDir

@@ +168,2 @@
>          result = FileCopyResult()
> +        have_symlinks = hasattr(os, 'symlink')

isn't that going to bite us when we use symlinks on windows, which, aiui, python doesn't support?

@@ +301,5 @@
> +                parents = set()
> +                while parent and parent != previous:
> +                    parents.add(parent)
> +                    previous = parent
> +                    parent = os.path.dirname(parent)

parent = f
previous=''
parents = set()
while True:
    parent = os.path.dirname(parent)
    parents.add(parent)
    if previous == parent:
        break
    previous = parent

? not sure which is clearer.

::: python/mozbuild/mozpack/test/test_copier.py
@@ +146,5 @@
> +        os.makedirs(self.tmppath('dest/foo'))
> +        dummy = self.tmppath('dummy')
> +        os.mkdir(dummy)
> +        link = self.tmppath('dest/foo/bar')
> +        os.symlink(dummy, link)

you should only do this if os.symlink exists.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #5)
> maybe add an ensureDir to util?

> ::: python/mozbuild/mozpack/copier.py
> @@ -29,5 @@
> > -
> > -    if not os.access(dir, os.W_OK):
> > -        umask = os.umask(0077)
> > -        os.umask(umask)
> > -        os.chmod(dir, 0777 & ~umask)
> 
> That access thing probably needs to be added to util's ensureParentDir

Let's do these things as followups.

> @@ +168,2 @@
> >          result = FileCopyResult()
> > +        have_symlinks = hasattr(os, 'symlink')
> 
> isn't that going to bite us when we use symlinks on windows, which, aiui,
> python doesn't support?

Probably.

Let's cross that bridge when we come to it? symlinks on Windows are hacky and will require support everywhere we do I/O. It isn't a trivial change.

> 
> @@ +301,5 @@
> > +                parents = set()
> > +                while parent and parent != previous:
> > +                    parents.add(parent)
> > +                    previous = parent
> > +                    parent = os.path.dirname(parent)
> 
> parent = f
> previous=''
> parents = set()
> while True:
>     parent = os.path.dirname(parent)
>     parents.add(parent)
>     if previous == parent:
>         break
>     previous = parent
> 
> ? not sure which is clearer.

I like your version.

> ::: python/mozbuild/mozpack/test/test_copier.py
> @@ +146,5 @@
> > +        os.makedirs(self.tmppath('dest/foo'))
> > +        dummy = self.tmppath('dummy')
> > +        os.mkdir(dummy)
> > +        link = self.tmppath('dest/foo/bar')
> > +        os.symlink(dummy, link)
> 
> you should only do this if os.symlink exists.

It wasn't in the context, but this whole tests is only enabled if the platform supports symlinks. There is a helper method on the test class that tests for this.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93e9a4784f0c

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93e9a4784f0c